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

Ruby: Include more nodes in {Hash,Array}LiteralCfgNode #14783

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 14, 2023

Removes the requirement that the Hash/Array reference must be qualified with ::.

@github-actions github-actions bot added the Ruby label Nov 14, 2023
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Nov 14, 2023
@hvitved hvitved marked this pull request as ready for review November 14, 2023 17:38
@hvitved hvitved requested a review from a team as a code owner November 14, 2023 17:38
Copy link
Contributor

@hmac hmac left a comment

Choose a reason for hiding this comment

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

I have a couple of questions:

  1. Will we erroneously include things like this?
module A
  class Array
    # def self.[] ...
  end
  
  def f
    Array[1, 2]
  end
end
  1. Can we add a test to show the new cases we catch?

@hvitved
Copy link
Contributor Author

hvitved commented Nov 15, 2023

  1. Will we erroneously include things like this?

I believe that class will have qualified name A::Array and not Array.

@hvitved
Copy link
Contributor Author

hvitved commented Nov 15, 2023

2. Can we add a test to show the new cases we catch?

There was already a test with updated expected output; is that ok?

@hvitved hvitved merged commit 75f42f4 into github:main Nov 16, 2023
21 checks passed
@hvitved hvitved deleted the ruby/hash-array-literal branch November 16, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants