feat: support gsub_file erroring if gsub doesn't change anything, and add gsub_file!
#877
Conversation
| def gsub_file!(path, flag, *args, &block) | ||
| config = args.last.is_a?(Hash) ? args.pop : {} | ||
|
|
||
| config[:error_on_no_change] = true |
There was a problem hiding this comment.
I'm ok with this feature but I don't think we should add a config to gsub_file to implement this. I think we can implement this by moving the logic to a private method that we can then add the option, or just use a block to change the behavior using the template method pattern.
There was a problem hiding this comment.
I've pushed up a change that is hopefully in the right direction - let me know what you think.
At this point I personally feel having the option on gsub_file is better because it's more flexible and feels like the DRYest way to do it already, but gsub_file! is most important to me so happy to go with your preference (and maybe I'm missing something about what you mean - I'm not sure what the "template method pattern" is at this point).
G-Rath
force-pushed
the
gsub_file-error-on-no-change
branch
from
4f92183
to
8eae746
Compare
|
@rafaelfranca can you let me know if you'd be interested in having bang methods for the other file manipulations? (either in this PR or another one) |
G-Rath
force-pushed
the
gsub_file-error-on-no-change
branch
from
8eae746
to
620b685
Compare
|
@rafaelfranca could I get a re-review on this please? |
|
@rafaelfranca would you mind re-reviewing this? we'd really like to get it landed and I think I've addressed your concerns 🙂 |
🌈
This is a straw-person solution for #874 - overall I think it's not bad but expect changes to be requested; in particular, the option name is pretty verbose so happy if someone wants to suggest an alternative though to me really its about having
gsub_file!.A bonus to this is that anything using
gsub_filealso supports this behaviour such ascomment_linesanduncomment_lines- I think it would be nice to have bang versions of such methods too but want to get the core behaviour/implementation nailed down first.Resolves #874