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

Railties updates for frozen string literals. #29891

Merged
merged 4 commits into from Aug 14, 2017

Conversation

pat
Copy link
Contributor

@pat pat commented Jul 22, 2017

Updates to railties for frozen-string-literals compatibility.

It's worth noting that there are dependencies on the following libraries that aren't yet compatible:

When the above patches are in play, then the tests pass with RUBYOPT="--enable-frozen-string-literal".

/cc @kirs given you're doing a lot of this work too!

@rails-bot
Copy link

r? @schneems

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

@pat
Copy link
Contributor Author

pat commented Jul 22, 2017

Also, @kirs: is this the last piece of the Rails puzzle that needs the pragma comment? Or are there other parts of Rails that still need some attention?

@kirs
Copy link
Member

kirs commented Jul 23, 2017

Great work!
Enabled parts are activesupport, activemodel, activejob, activerecord. I'm working on actioncable, and then there'll be actionmailer (#29896), actionpack (#29933), and actionview (#29897) left. Not counting railties that you're working on.

@kirs
Copy link
Member

kirs commented Jul 24, 2017

Can you add the frozen string literals to the changed files?
At least that's what @matthewd asked me for similar fixes in https://github.com/rails/rails/pull/29655/files#diff-53e65e5b02bb6a167c2270e40aa780fdL1

@koic
Copy link
Contributor

koic commented Aug 13, 2017

Hi @pat. Thanks for great work. It is a note related to this PR.

After rebasing with the latest master branch, you can remove 'railties/**/*' from Exclude of Style/FrozenStringLiteralComment in .rubocop.yml.

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

Please look when adding frozen string literal magic comments.

@kirs
Copy link
Member

kirs commented Aug 13, 2017

Looks like Railties is the last component that's left to complete the migration to frozen string literal 💯

@pat pat force-pushed the frozen-string-literals-railties branch from ab40a44 to 7f6a314 Compare August 14, 2017 17:58
@pat
Copy link
Contributor Author

pat commented Aug 14, 2017

Thanks @koic, @kirs. Latest commits I've just pushed include:

  • my original changes
  • all the pragma comments
  • the removal of the corresponding Rubocop exception
  • Some test tweaks now that the pragma comments are in place.

Let me know if you need anything further from me to get this merged in :)

@kirs
Copy link
Member

kirs commented Aug 14, 2017

LGTM ❤️

@rafaelfranca rafaelfranca merged commit 5ccdd0b into rails:master Aug 14, 2017
@pat
Copy link
Contributor Author

pat commented Aug 14, 2017

Thanks @rafaelfranca!

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.

None yet

6 participants