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

Remove const_get from StringIdentifierArgument target #427

Conversation

okuramasafumi
Copy link

[Fix #425]
const_get doesn't accept symbols that are something like :"Foo::Bar", and that's intended according to this issue. https://bugs.ruby-lang.org/issues/12319
Therefore, changing String to Symbol might not work as expected. As far as I know that's related to const_get only so we can remove it from the target of StringIdentifierArgument


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[Fix rubocop#425]
`const_get` doesn't accept symbols that are something like
`:"Foo::Bar"`, and that's intended according to this issue.
https://bugs.ruby-lang.org/issues/12319
Therefore, changing String to Symbol might not work as expected.
As far as I know that's related to `const_get` only so we can remove
it from the target of `StringIdentifierArgument`
@Earlopain
Copy link
Contributor

Earlopain commented Dec 21, 2023

I don't think this is the right solution.

This should just skip interpolated strings for const_get. In addition it's easily analyzable if the string literal contains double-colons to prevent a false positive.

From testing, this also applies to:

  • const_source_location
  • const_defined?

These don't:

  • public_const/private_const don't accept nested constants
  • deprecate_constant as well
  • Same for remove_const and set_const

@okuramasafumi
Copy link
Author

@Earlopain You are right. At least this change must cover all affected methods.
One concern is that the rule will be a little complex. Some might be confused about why it's not applied for some cases. We need to have a good documentation to explain.

@okuramasafumi
Copy link
Author

@Earlopain What about this case:

const_get("#{process(name)}Resource")

Here, process method returns a string that MIGHT contain ::. Is it analyzable?

@Earlopain
Copy link
Contributor

Perhaps in some trivial cases but generally no. I would just ignore those.

razumau added a commit to zendesk/property_sets that referenced this pull request Jan 3, 2024
Mostly it’s `Performance/StringIdentifierArgument`: methods like `instance_variable_get` or `send` should use symbols, not strings.

However, `const_get` does not accepts symbols with nested constants, like `Smth::Setting` (see this issue: rubocop/rubocop-performance#425, which might get fixed by rubocop/rubocop-performance#427). I’ve added `# standard:disable` in `spec/property_sets_spec.rb` until that fix is released.
@koic
Copy link
Member

koic commented Jan 3, 2024

I'll close this PR in favor of #430, which proposals a better resolution. Thank you.

@koic koic closed this Jan 3, 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.

The autocorrection for Performance/StringIdentifierArgument breaks code
3 participants