FrozenStringLiteralComment's 'when_needed' style is confusing

[mvz]

As I understand it, the 'when_needed' style only requires a frozen string literal comment if a call to String#freeze is present in the file. This seems weird, since once the comment is in place, the call is no longer needed.

Am I missing something?

$ rubocop -V
0.36.0 (using Parser 2.3.0.2, running on ruby 2.3.0 x86_64-linux)

[alexdowad]

@rrosenblum Any comment?

[rrosenblum]

@mvz sorry for not getting back to you sooner. You are right about the frozen string literal comment not being needed in a file if the file contains a call to String#freeze. I was trying to mimic the Style/Encoding cop, and I made use of some weak assumptions with that configuration that may not really make sense. The idea was to try and only add the comment to files that may need it. Targeting Ruby 2.3+ and files that contain a call to String#freeze or String#<< were the only criteria that we can check for. Even then, this does not catch all situations.

In hind site, I think this configuration is going to prove fairly useless and add more confusion than positive gain. I will put in a PR to remove both configurations, make the code function as the always configuration, and leave the cop disabled by default.

I don't know how many users actually dig through the documentation and read up on disabled cops. It might be nice if we had a way to enable this cop by default if the target Ruby version is 2.3+ or provide a message suggesting that this cop be enabled.

[alexdowad]

@rrosenblum You could just ensure that the file has at least one string literal in it.

[rrosenblum]

You could just ensure that the file has at least one string literal in it.

It seems so simple. I like it. What do you think is the percentage of files that contain no string literals compared to those that do? Is it worth it?

[alexdowad]

(Shrugs)

[rrosenblum]

Haha. We can wait for @bbatsov and others to chime in with their opinions.

[mvz]

@rrosenblum thanks for the explanation. I'll leave the cop disabled for now and keep an eye on the developments.

[rrosenblum]

Welcome. The always configuration works well if you want to use the cop.

[mvz]

@rrosenblum yes, I actually went ahead and used always instead. Worked like a charm.