rubocops/text: Enforce bin/"formula" instead of "#{bin}/formula"
#17826
Conversation
issyl0
force-pushed
the
no-interpolated-bin
branch
from
5ca8976
to
0530fb8
Compare
issyl0
force-pushed
the
no-interpolated-bin
branch
from
0530fb8
to
96400e0
Compare
|
When testing locally I'm not able to find any violations despite using |
|
What command are you running? It worked yesterday on the formula you suggested this be a thing for (with the change reapplied). Maybe I've done something wrong though. |
|
I tried a few formulae, but |
|
Ohhhh, I see what's happening. You did it right, I did it wrong. There's always a case I don't expect. |
|
So the |
|
Can we match |
Given we have a In that case we should also match all other methods that return a |
- Previously this only included the formula name.
- But, for example in tests, we have "#{bin}/ansible-test",
not just "#{bin}/ansible". So handle that too.
- I decided to make the error message better by extracting the
binary name from the interpolation, but I'm not sure it was worth it.
```
$ brew audit --strict ansible
ansible
* line 580, col 29: Use `bin/"ansible-test"` instead of `"#{bin}/ansible-test"`
Error: 1 problem in 1 formula detected.
```
issyl0
force-pushed
the
no-interpolated-bin
branch
from
43b7426
to
3713939
Compare
- I couldn't get https://docs.rubocop.org/rubocop-ast/node_pattern.html#param_name-for-named-parameters to work like it said it should (bad syntax in the node_matcher, apart from with `bin = false` which RuboCop complained about boolean args not being named), so here's a workaround.
|
Right, I made this handle "#{bin}/foo", "#{bin}/foo-bar" etc. I didn't go as far as all of "#{bin}/". If we need "#{bin}/foo_bar" or indeed all of "#{bin}/", or every Pathname, then we can iterate in a follow-up PR? |
Typechecking. Some functions will only take a string and this will break that. Especially where we use standard library functions (e.g. Open3). The IMO if we do this we need to play it safe and only flag known safe functions. |
…s fine - This would be weird to change because it's a string not a pathname passed to `shell_output`. - I had misunderstood #17826 (comment).
- This would be weird to change because it's a string not a pathname passed to `shell_output`. - I had misunderstood #17826 (comment).
- This would be weird to change because it's a string not a pathname passed to `shell_output`. - I had misunderstood #17826 (comment).
- This would be weird to change because it's a string not a pathname passed to `shell_output`. - I had misunderstood Homebrew#17826 (comment).
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?bin/"foo"over"#{bin}/foo"#17825.