Skip to content
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

NodePattern: Add Constants and keyword parameters #35

Merged
merged 2 commits into from
Jun 26, 2020

Conversation

marcandre
Copy link
Contributor

This PR allows use of constants in a NodePattern, and allows keyword parameters (in addition to the existing sequential parameters).

Example from the main gem

-       OPS = %w[== === != < > <= >= <=>].freeze
+       OPS = %i[== === != < > <= >= <=>].to_set.freeze

        def_node_matcher :useless_comparison?,
-                        "(send $_match {:#{OPS.join(' :')}} $_match)"
+                        "(send $_match %OPS $_match)"

This would also be faster (doing a single hash lookup instead of 8 comparisons, see #29)

The keyword parameters is for cases where positional parameters wouldn't be as clear, but also because I would like to make it possible to specify some keyword parameters at the definition too (upcoming PR)

For example, if one doesn't care to define a constant, one could write:

       def_node_matcher :useless_comparison?,
-                         "(send $_match {:#{OPS.join(' :')}} $_match)"
+                         "(send $_match %ops $_match)",
+                         ops: %i[== === != < > <= >= <=>].to_set.freeze

This could be overridden when calling the matcher, e.g.

useless_comparison?(node, ops: ...)

@marcandre marcandre force-pushed the nodepatternconstant branch 3 times, most recently from 03f81bc to eb90348 Compare June 14, 2020 07:11
@marcandre
Copy link
Contributor Author

PS: Still missing doc for %keyword_parameter form.

/cc @bbatsov

@marcandre marcandre force-pushed the nodepatternconstant branch 2 times, most recently from adb8211 to 397b3bb Compare June 14, 2020 22:08
@marcandre
Copy link
Contributor Author

PR finished, defaults added to def_node_pattern/search, documentation added too.

[source,ruby]
----
def_node_matcher :interesting_call?, '(send _ %method ...)',
method: Set[:transform_values, :transform_keys,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to accept arrays here as well and just implicitly convert them to sets. Seems to me this will result in nicer end user API, if it's more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you aware of my machiavelic plan to convert the whole world to Sets? ;-)

I like that currently the API is simple, we use <your object> === <node element>

It would be easy to convert the values to Sets at the level of def_node_matcher, but it would be a bad idea to do it at the matcher call level, so I'm afraid there would be confusion...

I should add a note in the doc that Array#=== will never work on a AST element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcandre marcandre force-pushed the nodepatternconstant branch 3 times, most recently from 0f05dcb to 805cf3d Compare June 16, 2020 06:46
@marcandre
Copy link
Contributor Author

Changelog entries added, PR ready as far as I'm concerned.

@marcandre marcandre requested a review from bbatsov June 16, 2020 06:49
@marcandre marcandre force-pushed the nodepatternconstant branch 2 times, most recently from 83c3566 to 2d9ebee Compare June 22, 2020 22:17
@marcandre
Copy link
Contributor Author

Rebased.

@marcandre marcandre mentioned this pull request Jun 25, 2020
@marcandre marcandre merged commit fe41cd6 into rubocop:master Jun 26, 2020
@marcandre marcandre deleted the nodepatternconstant branch June 26, 2020 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants