New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small Style
cops perf tweaks
#8359
Conversation
@@ -96,6 +98,14 @@ def on_send(node) | |||
|
|||
private | |||
|
|||
def access_modifier?(node) | |||
maybe_access_modifier?(node) && node.access_modifier? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but shouldn't it be Node#access_modifier?
itself that should be sped up instead of just this cop? I think just changing the order of the conditions in rubocop-ast
should give you most of the speed up, but I may be wrong
@@ -53,6 +55,8 @@ class FormatString < Cop | |||
PATTERN | |||
|
|||
def on_send(node) | |||
return unless FORMAT_METHODS.include?(node.method_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pattern that comes back very often for on_send
, right? It wouldn't be too hard to tweak Commissionner
so that a Cop could specify a list of method_name
to be called for (and not be bothered for others); it would be quite fast. Useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcandre I'd find that incredibly useful for many cops I write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool :-) Please check #8365 and let me know how well that fits what you'd like. It could be made more general but I think that on_send
and node.method_name
are by far the most important checks.
This broke the build ("Unnecessary disabling of Metrics/AbcSize"), now that the AbcSize has changed... Makes sense that the spec suite for the open PRs aren't re-run all the time 🤷♂️ I'm on it. |
Ran on
discourse
codebase:Before
After