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

Change symbol appearance for Rails macros #294

Merged
merged 4 commits into from
Mar 18, 2024
Merged

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Mar 18, 2024

This changes how we display symbols for Rails macros, so that:

  • It's easier to visual distinguish them from regular methods
  • They are a closer represention to how the macros are typically written in code.

@andyw8 andyw8 requested a review from a team as a code owner March 18, 2024 17:50
Comment on lines 164 to 166
unless arg_receiver.is_a?(Prism::ConstantReadNode) || arg_receiver.is_a?(Prism::ConstantPathNode)
name = ":#{name}"
end
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these conditions covered by the above already? What are we guarding against here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for it to be a CallNode but for the receiver to not be one of these two? That seems to be what's being guarded against

Copy link
Member

Choose a reason for hiding this comment

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

But don't we already guard this in the conditional above? If the receiver is not a ConstantReadNode or ConstantPathNode, then name will be nil and we'll skip, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's correct. It would be better to change the line where we're skipping, but if we do proceed with that, my question still stands about whether there's other cases worth factoring in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this can be removed now. I can't think of any other cases we would need.

@@ -161,8 +161,12 @@ def handle_all_arg_types(node, message)
name = arg_receiver.full_name if arg_receiver.is_a?(Prism::ConstantPathNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to your PR changes, but I just realized that these can also be merged, similar to here.

Edit: I'm referencing the code below, didn't select the whole thing when I made the initial comment

            name = arg_receiver.name if arg_receiver.is_a?(Prism::ConstantReadNode)
            name = arg_receiver.full_name if arg_receiver.is_a?(Prism::ConstantPathNode)

@andyw8 andyw8 enabled auto-merge (squash) March 18, 2024 20:17
@andyw8 andyw8 added the enhancement New feature or request label Mar 18, 2024
@andyw8 andyw8 merged commit b481c01 into main Mar 18, 2024
54 checks passed
@andyw8 andyw8 deleted the andyw8/display-rails-macros branch March 18, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants