Skip to content

Conversation

@luke-gru
Copy link
Contributor

such as:

p = Proc.new

This is now highlighted, and it wasn't before.

@mame mame requested a review from kddnewton September 17, 2024 13:23
@mame
Copy link
Member

mame commented Sep 17, 2024

@kddnewton Could you please review this change?

Copy link
Contributor

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

I don't think this is correct. You've made it so that .new is highlighted for Prism, but not highlighted for RubyVM::AbstractSyntaxTree. The behaviors should match. I think in this case it currently checks if nd_args && nd_args.first_lineno == nd_args.last_lineno. We can and should do the same check, and return nil if those conditions aren't met.

@luke-gru
Copy link
Contributor Author

Okay, thanks for the review. I've made the changes you mentioned.

@kddnewton
Copy link
Contributor

Sorry if I was unclear, but this appears to have gone in the opposite direction from what I said. Instead of returning nil if those conditions are met, now it tries to highlight the name. I'm saying we should keep the previous behavior for RubyVM::AbstractSyntaxTree and switch Prism to match, which would be returning nil in the case that there are no arguments.

@kddnewton kddnewton force-pushed the fix_prism_call_with_noargs branch from c474147 to 3a0b2eb Compare September 24, 2024 13:26
such as:

  p = Proc.new

This now matches the RubyVM::AbstractSyntaxTree behavior, which is
not to highlight anything.
@kddnewton kddnewton force-pushed the fix_prism_call_with_noargs branch from 3a0b2eb to d5c592a Compare September 24, 2024 13:26
@kddnewton kddnewton merged commit 452f786 into ruby:master Sep 24, 2024
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.

3 participants