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

Update for frozen-string-literal friendliness. #249

Merged
merged 2 commits into from
Oct 5, 2017

Conversation

pat
Copy link

@pat pat commented Oct 5, 2017

One minor change that keeps all tests green when frozen string literals are in play in MRI 2.4+.

I've done this testing by adding the pragma comment to all Ruby files in lib and test locally, but have not included those changes in this PR. I'm happy to add them in if you'd like, I just don't want to presume whether that's preferred or not.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @gsamokovarov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@gsamokovarov
Copy link
Collaborator

Hey @pat, thanks for the work. I don't mind the the frozen string literals, as long as they keep the tests green.

This covers every ruby and rake file in lib and test.
@pat
Copy link
Author

pat commented Oct 5, 2017

@gsamokovarov just added a commit with all the pragma comments, plus some changes to line numbers in the integration test (as they were all off by two due to the pragma comment). Tests are still green locally, would expect that to be the case on Travis as well.

The one catch: there's nothing that'll keep those comments in place in the current files, nor ensure they're present for new files. In other projects I find others use Rubocop to enforce this, but that's just something to consider. At the very least, having these comments will ensure that any future invalid uses of frozen string literals will cause test failures in the current files.

@gsamokovarov gsamokovarov merged commit ec96734 into rails:master Oct 5, 2017
@gsamokovarov
Copy link
Collaborator

Thanks @pat, I'm merging this as is. The rubocop configuration is a good idea and any PRs in that regard are welcome. We shouldn't diverge from the rails/rails rubocop settings, though.

Anyway, thanks for the contribution! ❤️

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

3 participants