search

rubocop / rubocop-ast

RuboCop's AST extensions and NodePattern functionality
https://docs.rubocop.org/rubocop-ast
MIT License
112 stars 53 forks source link

Document named captures #320

Closed Earlopain closed 1 month ago

Earlopain commented 1 month ago

It's a nifty feature currently not mentioned in the docs. Hash keys can be interpolated which would then not make this match anymore but for demonstration purposes this is the best example I could come up with.

cc @dvandersluis

Unrelated to this, I've added ruby code styling to a code block that was previously missing it.

dvandersluis commented 1 month ago

Something to point out is that you can do this without actually making them captures, ie

(pair
  (_ _key)
  (_ _key))

will also work to ensure the same value in both spots, without capturing _key.

Also (as discussed on the discord), this also works: ..._foo (multiple nodes as a named variable which can be referenced -- captures don't seem to work properly though!) . @marcandre this isn't documented or tested and I'm not sure if it's purposeful?

Earlopain commented 1 month ago

without capturing _key

Oh, interesting. It's probably better to move it to https://docs.rubocop.org/rubocop-ast/node_pattern.html#_-for-any-single-node then. If the ..._name pattern is intentionally supported it'll not be far of from where it was introduced.

marcandre commented 1 month ago

Something to point out is that you can do this without actually making them captures, ie

(pair
  (_ _key)
  (_ _key))

will also work to ensure the same value in both spots, without capturing _key.

Correct. Thanks for the PR 🚀

Also (as discussed on the discord), this also works: ..._foo (multiple nodes as a named variable which can be referenced -- captures don't seem to work properly though!) . @marcandre this isn't documented or tested and I'm not sure if it's purposeful?

That's incorrect. The lexer is often not space sensitive. In this case, ..._foo is actually treated the same as ... _foo, so _foo refers to a single last element (which must exist for a match), not to the variable number of remaining elements (which could be 0).

Would it be helpful to support named "rest"? I think it should look like ...name and $...name. I'd have to modify a bit the lexer and the parser, and maybe compiler to, not sure. If so, we can create a separate issue for the feature request; I'll close this one.

marcandre commented 1 month ago

Note: I'm not too sure how to force the update of the docs, but I'm planning on resolving #319 soonish and will release that.

dvandersluis commented 1 month ago

That's incorrect. The lexer is often not space sensitive. In this case, ..._foo is actually treated the same as ... _foo, so _foo refers to a single last element (which must exist for a match), not to the variable number of remaining elements (which could be 0).

Would it be helpful to support named "rest"? I think it should look like ...name and $...name. I'd have to modify a bit the lexer and the parser, and maybe compiler to, not sure. If so, we can create a separate issue for the feature request; I'll close this one.

Got it, that makes sense. I was pointing it out as an observation I had when playing with the pattern tester, but I didn't look into it closer. Thanks for the explanation.

I think it'd make sense in the abstract to support ...name to go with _name but I don't know if there's a specific requirement for it.

marcandre commented 1 month ago

I think it'd make sense in the abstract to support ...name to go with _name but I don't know if there's a specific requirement for it.

Ok cool. Maybe we could mention in the docs that it's not supported but to open an issue if there's ever an actual use case?