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

Enforce frozen_string_literal: true #589

Merged
merged 2 commits into from Nov 9, 2023

Conversation

sambostock
Copy link
Contributor

The default EnforcedStyle is always, which enforces that all files have a # frozen_string_literal comment, but doesn't care if it's true or false. always_true enforces true, which we want. Any existing files can simply be added to a TODO list or disable the cop.

The default `EnforcedStyle` is `always`, which enforces that all files have a `# frozen_string_literal` comment, but doesn't care if it's `true` or `false`. `always_true` enforces `true`, which we want. Any existing files can simply be added to a TODO list or disable the cop.
@sambostock sambostock requested a review from a team as a code owner November 9, 2023 20:44
@github-actions github-actions bot added the config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops label Nov 9, 2023
This automated commit dumps the contents of the full RuboCop config.
[dependabot skip]
@sambostock sambostock merged commit 3a104d9 into main Nov 9, 2023
20 checks passed
@sambostock sambostock deleted the enforce-frozen-string-literal-true branch November 9, 2023 20:55
@andyw8
Copy link
Contributor

andyw8 commented Nov 9, 2023

I think this is problematic because we override SafeAutoCorrect to be true.

If you have a file such as:

# frozen_string_literal: false

a = "x"
a << "y"

and run rubocop -a (i.e. 'safe' autocorrect), it will change false to true which will result in a runtime error.

@sambostock
Copy link
Contributor Author

Hmmm... wouldn't that be fine, since the developer would have to triage it in their PR anyway? I think if we have that false, then the dev would have to manually add the comment, rather than the something like VS Code adding it automatically.

@sambostock
Copy link
Contributor Author

Arguably, it was always possible to write that file without a comment, and run the autocorrect which would default to frozen_string_literal: true I believe, which would be the same problem. The only difference being that modifying it on existing files could break production code, but it would go through PR review, so should be caught.

@andyw8
Copy link
Contributor

andyw8 commented Nov 9, 2023

Ok, yeah, it's probably not a big deal, just feels a little off to label it as safe when it's not.

@sambostock
Copy link
Contributor Author

Yeah, I agree. I tracked down the PR that originally made that change though, and it looks like it's intentional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants