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

Add frozen_string_literal: true to all files #1745

Closed

Conversation

dillonwelch
Copy link

@dillonwelch dillonwelch commented Mar 25, 2018

I see that the option is enabled in .travis.yml for loading ruby but this doesn't help when including the gem in a project.

@flavorjones
Copy link
Member

The magic comment was added to a few generated files, I'm curious how you did this (maybe with https://github.com/Invoca/magic_frozen_string_literal) and whether you considered how those generated files would stay updated?

@dillonwelch
Copy link
Author

Yeah, I used magic_frozen_string_literal. I see that lib/nokogiri/css/tokenizer.rb and lib/nokogiri/css/parser.rb have comments indicating they are auto generated. I did not notice this until you pointed it out. Are there any others I missed? Where can I go to edit the auto generator script to include the pragma?

@flavorjones flavorjones added this to the v1.11.0 milestone Jan 6, 2019
flavorjones added a commit that referenced this pull request Nov 25, 2019
flavorjones added a commit that referenced this pull request Nov 25, 2019
flavorjones added a commit that referenced this pull request Nov 25, 2019
driven by rubocop, see previous commit

related to #1745
flavorjones added a commit that referenced this pull request Nov 25, 2019
@flavorjones
Copy link
Member

Hi, @oniofchaos. Sorry for the delay here. I've updated the test suite to have RuboCop add the magic comment to all lib files, to future-proof the concept in this PR. I've credited you in the changelog, but won't be merging this commit since it's been superseded by the RuboCop automation.

Thanks so much for your work here, I wouldn't have gotten around to this without your help.

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.

2 participants