rubocop: generate_completions DSL #13553
Conversation
Not sure, I guess I haven't tried. I think you could probably write a pattern that checks for multiple instances right after each other. That would probably be pretty easy to implement. You also could probably do something like this (not tested at all): def audit_formula(_node, _class_node, _parent_class_node, body_node)
install = find_method_def(body_node, :install)
completion_nodes_to_fix = []
correctable_shell_completion_node(install) do |node, shell, base_name, binary, cmd, shell_parameter|
...
# Instead of calling `offending_node` and `problem`:
completion_nodes_to_fix << [node, replacement]
end
completion_nodes_to_fix[0...-1].each do |node, _|
offending_node(node)
# The wording here is terrible but is just an example
problem "Use a single `generate_completions` call to generate completions" do |corrector|
corrector.replace(@offensive_node.source_range, "")
end
end
# Create the single `generate_completions` call
offending_node(completion_nodes_to_fix.last.first)
problem "Use `#{replacement}` instead of `#{@offensive_node.source}`." do |corrector|
corrector.replace(@offensive_node.source_range, replacement)
end
endThat's just an example, I haven't thought through the logic very thoroughly so there might be a better way to do it. If those don't work, you could probably write a separate rubocop that combines all of the individual |
Ah yes, I forgot to mention tests in my initial comment. RuboCop tests are, luckily, pretty easy to make. I'd recommend looking at the |
|
Thank you for the feedback and helpful example code! I'll update the PR with the proposed changes as soon as I refactored the DSL PR, and will also include Unit Tests when doing so. |
|
Hello there! I have updated the PR to reflect the changes made in the DSL PR, and also included some other fixes. I have also added another Cop to combine multiple calls with different shells into one. The only thing I haven't been able to figure out is how to instruct the corrector to also remove the empty lines left behind, as can be seen in the homebrew/core example PR. I have looked into the As soon as that is figured out, I will also add Unit tests. |
Sure, done! |
|
I have now extended the tests. 😄 Now that Homebrew/homebrew-core#105707 is also merged, this should be good to go if CI passes 🎉 |
|
Yeah, I'm gonna need some help with that CI error... I can reproduce the failure if I have a formula containing only an E.g. class Foo < Formula
def install
(bash_completion/"foo").write Utils.safe_popen_read(bin/"foo", "completions", "bash")
end
end |
|
Also, the cask audit test failures should be unrelated. |
|
Or: should we simply scope this cop to the core tap? The run for all core formulae was fine. |
|
Yay, all green now! (except for the unrelated tests) Thanks again! That should be everything now for the new DSL 🎉 |
|
@SMillerDev the current CI issues (which are occurring for me locally even on the Weirdly, I can see the errors if I run |
|
I have no idea, it passed for me and in CI. |
|
Failures likely fixed by #13813. |
Curiously, they are still present in the latest CI run 🧐 |
Yes, probably. |
e.g. ˚jrsonnet˚ needs this
needed for e.g. àwscli`, `hatch`, `pipenv`
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
max-ae
force-pushed
the
generate-completions-dsl-rubocop
branch
from
89bb77e
to
088dce0
Compare
|
Thanks so much for all of your work on this, @max-ae! 🎉 |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?related to #13536
needs Homebrew/homebrew-core#105707
cc @Rylan12
I have achieved to implement a preliminary RuboCop, but I will appreciate any feedback, since I'm sure there's room for improvement.
The Cop is able to correct
but I have been unable to come up with a solution to get and replace with all shells whilst only walking the AST once.
The Cop also warns for the
pattern, but has no corrector, as discussed in #13536.
Please feel free to push any commits improving my implementation 😄