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

Improve docs #265

Merged
merged 6 commits into from May 23, 2023
Merged

Improve docs #265

merged 6 commits into from May 23, 2023

Conversation

pirj
Copy link
Member

@pirj pirj commented May 21, 2023

This:

  • adds a smooth introduction to AST, basic node pattern usages
  • adds a section for negation syntax
  • small fixes

fixes #228

`RuboCop::ProcessedSource` is from `rubocop`.
* it will match `sum(2.0, 3)`, as the first argument is of a `float` type
* it will not match `sum(2, 3)`, as the first argument is of an `int` type

NOTE: Negation operator works with other node pattern syntax elements, `{}`, `[]`, `()`, `$`, but not with `<>`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm uncertain if this is intentional.
(send nil? :foo <int ...>) would work to match a foo call with at least one integer literal.
(send nil? :foo !<int ...>) would not work to match a foo call with no literal integer arguments. Is there a more canonical way to achieve that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's more complex than that. You can negate a pattern that targets a single element (e.g. !{int | sym}) but not other pattern (e.g. !{int int | sym sym}).
Not sure why you wrote it doesn't work with $

Copy link
Member Author

@pirj pirj May 23, 2023

Choose a reason for hiding this comment

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

Got it.
I understand that with variable number of elements like {int | sym sym} and <int ...> it's an easily justifiable behaviour.

I guess (send _ !<true false>) is more tricky due to the hidden number of elements ambiguity? It seems like it would match if there are no arguments (foo()), a single non-boolean argument (foo(1)), and multiple arguments as well, which just would open space for more confusion.

Not sure why you wrote it doesn't work with $

Because I didn't! 😄

Thanks for the clarification! I've updated this section.

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for improving the docs. I have a few comments

docs/modules/ROOT/pages/node_pattern.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/node_pattern.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/node_pattern.adoc Outdated Show resolved Hide resolved
* it will match `sum(2.0, 3)`, as the first argument is of a `float` type
* it will not match `sum(2, 3)`, as the first argument is of an `int` type

NOTE: Negation operator works with other node pattern syntax elements, `{}`, `[]`, `()`, `$`, but not with `<>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's more complex than that. You can negate a pattern that targets a single element (e.g. !{int | sym}) but not other pattern (e.g. !{int int | sym sym}).
Not sure why you wrote it doesn't work with $

@pirj pirj force-pushed the add-sequence-introduction branch 3 times, most recently from 1788b18 to e2a6fa5 Compare May 23, 2023 15:21
pirj added 4 commits May 23, 2023 18:59
Without `--legacy`, it would return `kwargs`, not `hash`:

    (send
      (const nil :Comment) :new
      (kwargs
        (pair
          (sym :user)
          (send nil :current_user))))
@pirj pirj force-pushed the add-sequence-introduction branch from 075a1f2 to 02d78f0 Compare May 23, 2023 15:59
Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Very nice, thank you

docs/modules/ROOT/pages/node_pattern.adoc Outdated Show resolved Hide resolved
@marcandre marcandre merged commit 0677018 into rubocop:master May 23, 2023
18 of 19 checks passed
@pirj pirj deleted the add-sequence-introduction branch May 23, 2023 20:03
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.

Basic documentation of Node Pattern
2 participants