[Fix #7426] Add `always_true` style to Style/FrozenStringLiteralComment #7435

parkerfinch · 2019-10-16T03:38:40Z

This adds another style to Style/FrozenStringLiteralComment that
enforces that the comment is always enabled, that is, requiring that
string literals are always frozen. The default style, always,
enforces that the comment exists but does not enforce that the comment
is enabled.

@gfyoung does this line up with what you're thinking? Feel free to make any changes to this as you see fit and push up something that aligns more closely with your idea!

I had most of this put together before I saw @olliebennett's suggestion to name this alwaystrue -- I'm happy to change the name here, let me know if you have thoughts!

I am an infrequent contributor, please let me know if I forgot any steps or could make this easier to take a look at!

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

buehmann · 2019-10-16T04:05:24Z

lib/rubocop/cop/style/frozen_string_literal_comment.rb

        def remove_comment(corrector, node)
          corrector.remove(range_with_surrounding_space(range: node.pos,
                                                        side: :right))
        end

+        def enable_comment(corrector)
+          # require "pry"
+          # binding.pry


Old debugging code

Gah, thanks!

gfyoung

@parkerfinch : Thanks for opening this! This looks pretty good.

I think I'm leaning more toward what @olliebennett had suggested because it makes it clear that we are enforcing #frozen_string_literal: true. Maybe use enforcetrue instead to be even more explicit.

parkerfinch · 2019-10-16T12:14:51Z

Thanks for taking a look @gfyoung, made the suggested name change!

bbatsov · 2019-10-21T07:44:30Z

I like the idea in general. I'm thinking that always_true might be more in line with the current naming.

gfyoung · 2019-10-21T07:48:17Z

I like the idea in general. I'm thinking that always_true might be more in line with the current naming.

@bbatsov : Sounds good. Whatever term is most intuitive!

parkerfinch · 2019-10-21T22:56:51Z

Thanks @bbatsov , pushed that name change!

gfyoung · 2019-10-21T23:33:18Z

lib/rubocop/cop/style/frozen_string_literal_comment.rb

+      #
+      #   module Bar
+      #     # ...
+      #   end
      class FrozenStringLiteralComment < Cop
        include ConfigurableEnforcedStyle
        include FrozenStringLiteral
        include RangeHelp

        MSG = 'Missing magic comment `# frozen_string_literal: true`.'


Should we change this one since we don't strictly require # frozen_string_literal: true unless always_true is passed in as the configuration?

Hrrmm -- I see how the current comment could be misleading, but I'm not sure what a better solution is. Maybe if we made it clear that we weren't giving the full text of the comment, e.g. Missing magic "frozen_string_literal" comment? But I think there's some value in giving the exact syntax expected for the comment.

Do you have thoughts on what to replace this with?

But I think there's some value in giving the exact syntax expected for the comment

If you're going to give exact syntax, then you should give all of the accepted values for clarity to the end user (clarity is what we're ultimately aiming for when I opened #7426). Otherwise, I would say Missing magic "frozen_string_literal" comment and allow them to look it up in the docs, where we have more space to elaborate.

Yeah, probably there should be different messages for the different enforced styles. The current message is appropriate only for the new always_true style.

Addressed in #7634

parkerfinch · 2019-10-26T18:52:07Z

@gfyoung @bbatsov I pushed up the message change for the current cop -- let me know if there's anything else I can do to help out here!

gfyoung · 2019-10-31T05:13:03Z

@parkerfinch : These changes look great! Just waiting on @bbatsov to give further feedback or merge.

parkerfinch · 2019-11-04T14:46:02Z

@bbatsov I think this is good to go, let me know if there's anything I can do to make this easier to review!

lib/rubocop/cop/style/frozen_string_literal_comment.rb

This adds another style to Style/FrozenStringLiteralComment that enforces that the comment is always enabled, that is, it requires that string literals are always frozen. The default style, `always`, enforces that the comment exists but does not enforce that the comment is enabled. This also differentiates the offense message for the `always` and `always_true` styles. It changes the offense message when using the `always` style to be less specific, since the old version of the message implied that the only acceptable value of the frozen_string_literal comment was `true`. In fact, in the `always` style, either `true` or `false` is acceptable. The new `always_true` style uses that preexisting message, since when using that style it is required that the value of the frozen_string_literal comment is `true`.

parkerfinch · 2019-12-02T18:00:10Z

@bbatsov @koic Think this is good to merge?

gfyoung · 2019-12-03T08:59:25Z

I would definitely love for this feature to be merged! Thanks for all the hard work @parkerfinch!

bbatsov · 2019-12-31T11:31:39Z

The changes look good, but your branch has to be rebased on top of the current master branch due to merge conflicts.

bbatsov · 2019-12-31T11:33:31Z

lib/rubocop/cop/style/frozen_string_literal_comment.rb

-        MSG = 'Missing magic comment `# frozen_string_literal: true`.'
+        MSG_MISSING_TRUE = 'Missing magic comment `# frozen_string_literal: '\
+                           'true`.'
+        MSG_MISSING = 'Missing magic "frozen_string_literal" comment.'


It'd be nice if all those messages employed consistent wording. Now they look a bit weird.

pirj · 2020-01-02T19:02:51Z

Before this gets merged, I'd like to coin my 2¢.

For dealing with string literals I see those sane options:

Always freeze by including #frozen_string_literal: true directive. Good when you know string literals you define won't be passed to third-party code that will attempt to mutate them.
Freeze only when needed, e.g. include only if there are string literals in the file. Same use case as above. The cop would suggest to include when there are string literals and to remove when there are no string literals.
An option to this might be a hysteresis when pragma is suggested to be added when there are string literals in the file, but it's not suggested to remove the pragma when there are no string literals.
Don't freeze. Nothing to be done except for disabling the cop or setting enforced style to never, since freezing is disabled by default. Used when string literals might be used in third-party code attempting to mutate them.
A pretty edge case is to selectively disable, but this can be done by using Exclude list, or locally disabling cop for those files. Is this any worse than frozen_string_literal: false?
Don't freeze selectively. For those cases when select files are known to be a source of string literals that are passed to third-party code that might attempt to mutate them. When the interpreter is being run with a flag (ruby --enable frozen-string-literal), and freezing is on by default, but for some files freezing needs to be explicitly turned off.

With that, I'm trying to point out that always_true should probably become what always is now, and there's no real use case for always with its current behaviour.

Hope you'll find this convincing enough to change how always behaves, and I can handle adding when_needed and only_when_needed.

gfyoung · 2020-01-07T09:45:47Z

Opened #7634 to bring this over the finish line

parkerfinch · 2020-01-07T14:48:37Z

Closing in favor of #7634

@parkerfinch

FROM #7435: This adds another style to Style/FrozenStringLiteralComment that enforces that the comment is always enabled, that is, it requires that string literals are always frozen. The default style, `always`, enforces that the comment exists but does not enforce that the comment is enabled. This also differentiates the offense message for the `always` and `always_true` styles. It changes the offense message when using the `always` style to be less specific, since the old version of the message implied that the only acceptable value of the frozen_string_literal comment was `true`. In fact, in the `always` style, either `true` or `false` is acceptable. The new `always_true` style uses that preexisting message, since when using that style it is required that the value of the frozen_string_literal comment is `true`. === Same changes as the original PR with merge conflicts addressed. Original author: @parkerfinch Closes #7426 Closes #7435

The wording regarding missing and unnecessary string literals are aligned with each other. Addresses: #7435 (comment)

buehmann reviewed Oct 16, 2019

View reviewed changes

gfyoung reviewed Oct 16, 2019

View reviewed changes

parkerfinch changed the title ~~[Fix #7426] Add frozen style to Style/FrozenStringLiteralComment~~ [Fix #7426] Add enforcetrue style to Style/FrozenStringLiteralComment Oct 16, 2019

parkerfinch force-pushed the enforce-frozen-string-literal-true branch from b5acfaf to 79f743c Compare October 17, 2019 17:17

parkerfinch force-pushed the enforce-frozen-string-literal-true branch from 79f743c to 601a0c6 Compare October 21, 2019 22:53

parkerfinch changed the title ~~[Fix #7426] Add enforcetrue style to Style/FrozenStringLiteralComment~~ [Fix #7426] Add always_true style to Style/FrozenStringLiteralComment Oct 21, 2019

gfyoung reviewed Oct 21, 2019

View reviewed changes

parkerfinch force-pushed the enforce-frozen-string-literal-true branch 2 times, most recently from ee83527 to 59a2a86 Compare October 25, 2019 18:37

parkerfinch force-pushed the enforce-frozen-string-literal-true branch 2 times, most recently from 650c715 to 2ff6b74 Compare October 28, 2019 18:57

parkerfinch force-pushed the enforce-frozen-string-literal-true branch from 2ff6b74 to 6c35dfd Compare November 4, 2019 14:20

koic reviewed Nov 15, 2019

View reviewed changes

lib/rubocop/cop/style/frozen_string_literal_comment.rb Show resolved Hide resolved

parkerfinch force-pushed the enforce-frozen-string-literal-true branch from 6c35dfd to 65e2e30 Compare November 15, 2019 13:50

parkerfinch force-pushed the enforce-frozen-string-literal-true branch from 65e2e30 to b586912 Compare December 2, 2019 17:52

bbatsov reviewed Dec 31, 2019

View reviewed changes

gfyoung mentioned this pull request Jan 7, 2020

Enforce frozen string literal true #7634

Merged

8 tasks

parkerfinch closed this Jan 7, 2020

parkerfinch deleted the enforce-frozen-string-literal-true branch January 7, 2020 14:48

bbatsov pushed a commit that referenced this pull request Jan 11, 2020

Align error messages about frozen string literal

612adbb

The wording regarding missing and unnecessary string literals are aligned with each other. Addresses: #7435 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #7426] Add `always_true` style to Style/FrozenStringLiteralComment #7435

[Fix #7426] Add `always_true` style to Style/FrozenStringLiteralComment #7435

parkerfinch commented Oct 16, 2019

buehmann Oct 16, 2019

parkerfinch Oct 16, 2019

gfyoung left a comment

parkerfinch commented Oct 16, 2019

bbatsov commented Oct 21, 2019

gfyoung commented Oct 21, 2019

parkerfinch commented Oct 21, 2019

gfyoung Oct 21, 2019 •

edited

parkerfinch Oct 22, 2019

gfyoung Oct 22, 2019 •

edited

bbatsov Oct 23, 2019

gfyoung Jan 7, 2020

parkerfinch commented Oct 26, 2019

gfyoung commented Oct 31, 2019

parkerfinch commented Nov 4, 2019

parkerfinch commented Dec 2, 2019

gfyoung commented Dec 3, 2019

bbatsov commented Dec 31, 2019

bbatsov Dec 31, 2019

pirj commented Jan 2, 2020 •

edited

gfyoung commented Jan 7, 2020

parkerfinch commented Jan 7, 2020

[Fix #7426] Add always_true style to Style/FrozenStringLiteralComment #7435

[Fix #7426] Add always_true style to Style/FrozenStringLiteralComment #7435

Conversation

parkerfinch commented Oct 16, 2019

buehmann Oct 16, 2019

Choose a reason for hiding this comment

parkerfinch Oct 16, 2019

Choose a reason for hiding this comment

gfyoung left a comment

Choose a reason for hiding this comment

parkerfinch commented Oct 16, 2019

bbatsov commented Oct 21, 2019

gfyoung commented Oct 21, 2019

parkerfinch commented Oct 21, 2019

gfyoung Oct 21, 2019 • edited

Choose a reason for hiding this comment

parkerfinch Oct 22, 2019

Choose a reason for hiding this comment

gfyoung Oct 22, 2019 • edited

Choose a reason for hiding this comment

bbatsov Oct 23, 2019

Choose a reason for hiding this comment

gfyoung Jan 7, 2020

Choose a reason for hiding this comment

parkerfinch commented Oct 26, 2019

gfyoung commented Oct 31, 2019

parkerfinch commented Nov 4, 2019

parkerfinch commented Dec 2, 2019

gfyoung commented Dec 3, 2019

bbatsov commented Dec 31, 2019

bbatsov Dec 31, 2019

Choose a reason for hiding this comment

pirj commented Jan 2, 2020 • edited

gfyoung commented Jan 7, 2020

parkerfinch commented Jan 7, 2020

[Fix #7426] Add `always_true` style to Style/FrozenStringLiteralComment #7435

[Fix #7426] Add `always_true` style to Style/FrozenStringLiteralComment #7435

gfyoung Oct 21, 2019 •

edited

gfyoung Oct 22, 2019 •

edited

pirj commented Jan 2, 2020 •

edited