rubocop / rubocop-performance

An extension of RuboCop focused on code performance checks.
https://docs.rubocop.org/rubocop-performance
MIT License
670 stars 79 forks source link

Remove `const_get` from `StringIdentifierArgument` target #427

Closed okuramasafumi closed 8 months ago

okuramasafumi commented 8 months ago

[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:

Earlopain commented 8 months ago

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:

These don't:

okuramasafumi commented 8 months ago

@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 commented 8 months ago

@Earlopain What about this case:

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

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

Earlopain commented 8 months ago

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

koic commented 8 months ago

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