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

Use frozen string literal in Active Storage #30211

Merged
merged 1 commit into from
Aug 13, 2017

Conversation

koic
Copy link
Contributor

@koic koic commented Aug 12, 2017

Summary

Extends Style/FrozenStringLiteralComment RuboCop rule to activestorage/.

I also fixed that the "can not modify frozen String" error does not occur in rails/activestorage tests.

Related PR #29897.

/Cc @kirs

Other Information

There is the following Performance/UnfreezeString cop recently merged into RuboCop.

rubocop/rubocop#4586

It seems that String#+@ is superior to String#dup, but this PR does not using it because support target can not be limited to Ruby 2.3 or higher.

@rails-bot
Copy link

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@kirs
Copy link
Member

kirs commented Aug 12, 2017

LGTM. Thanks for adding the rule for ActiveStorage!

I'm thinking that maybe now in .rubocop.yml we should use Exclude components that are not frozen string ready yet, instead of listing them under Include.

@rafaelfranca
Copy link
Member

👍 to @kirs idea of flipping the whitelist to a blacklist. Could you do that?

@rafaelfranca rafaelfranca added this to the 5.2.0 milestone Aug 12, 2017
@koic
Copy link
Contributor Author

koic commented Aug 13, 2017

Thanks for your review and advise! I fixed it at 1822e04.

@matthewd
Copy link
Member

That's not quite what @kirs meant.. we should end up with something more like:

Style/FrozenStringLiteralComment:
  Enabled: true
  EnforcedStyle: always
  Exclude:
    - 'railties/**/*'
    - 'actionview/test/**/*.builder'
    - 'actionview/test/**/*.ruby'
    - 'actionpack/test/**/*.builder'
    - 'actionpack/test/**/*.ruby'

(because AFAICS, railties is the only component not currently listed in the Include section)

@koic
Copy link
Contributor Author

koic commented Aug 13, 2017

Oops! I was misunderstanding. Thanks @matthewd.

I reverted 1822e04.

Besides railties, top level components such as guides, tools, tasks, and Gemfile... also require string literal freezing. I'd like to do it by another PR because the target becomes wider. What do you think?

@kirs
Copy link
Member

kirs commented Aug 13, 2017

Let's do it in the separate PR as you suggest.

@matthewd matthewd merged commit 4ec5b76 into rails:master Aug 13, 2017
@koic koic deleted the frozen_activestorage branch August 13, 2017 12:16
@koic
Copy link
Contributor Author

koic commented Aug 13, 2017

Thank you, all!

@koic koic mentioned this pull request Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants