Default for Style/FormatStringToken

[marcandre]

Why is our default for Style/FormatStringToken annotated?

I've always personally used the "template" style and I wonder why we have to worry about these suffixes and alternate format?

[marcandre]

I believe "template" style is also Rails' standard, and our default is creates false positives: #7452

[koic]

If this rule is most important to specify to named format string tokens (keyword arguments), then I have one idea. E.g.:

# bad
format('%s', 'Hello')

# good
format('%<greeting>s', greeting: 'Hello')
format('%{greeting}', greeting: 'Hello')

I think it can accept both EnforcedStyle: annotated and EnforcedStyle: template styles by default.

So I think this cop can probably be updated to accept multiple EnforcedStyle configuration using AllowMultipleStyles: true to better default configuration.

# .rubocop.yml
Style/FormatStringToken:
  EnforcedStyle:
    - annotated
    - template

cf: #6649

[marcandre]

@koic This seems fine with me, although I would still prefer a single style (template).

For exaple, I'm reviewing #8844 and there is a change like:

-       MSG = '$%{backref} is out of range (%{count} Regexp capture groups detected).'
+       MSG = '$%<backref>s is out of range (%<count>s Regexp capture groups detected).'

and I much prefer the first version. Note that backref is an integer, but %<backref>i or %<backref>s have the same result for integers anyways...

[marcandre]

cc/@bbatsov

[marcandre]

ping @bbatsov

[bbatsov]

I've always personally used the "template" style and I wonder why we have to worry about these suffixes and alternate format?

Simply because I felt this resulted in the most maitainable code, as it features more info. I knew relatively few people even knew they could name the print placeholders with names and their type and I thought this was a good practice worth promoting. I don't recall anything complaining about the default, even if it was uncommon, so I can only hope this got traction from RuboCop. :-)

I think it can accept both EnforcedStyle: annotated and EnforcedStyle: template styles by default.

That's an option, but I'm not fond of mixing styles for no reason. I do like one recent suggestion to add some limit under which it's ok not to name things, as the benefit is most pronnounced with several placeholders anyways.

[marcandre]

There are two orthogonal questions here: naming things (I'm all for it, at least for more than one placeholder) and typing / format. It's really the second I'm questioning.

Note that type "s" features no additional info, as the argument can be any type and to_s is called on it.

[bbatsov]

Well, it helps you figure out the types of the other arguments for format, but it's obviously subjective weather that adds value or not. I agree that the names are definitely the more important part here.

[marcandre]

So in conclusion, should I understand you'd rather keep the default as is?

[bbatsov]

I don't have a strong preference, but as the users are not complaining I'd rather not introduce any headaches for them, by changing the default. That's my only consideration. If you want to check whether a lot of people are changing the default (e.g. by doing some scan of publicly available RuboCop config files) and it turns out that's the case - then it'd be fine by me.

[marcandre]

I understand the concern.

I think format strings are not that much used so I doubt I would see much config changes even if I knew where to look or public config files.

If we added autocorrection from annotated to template style though, impact on users would be minimal (change the config, or agree with new default and autocorrect).

Concern for users should be weighed with the fact that we currently create issues for Rails routes, among others #7452

[pirj]

Checked on real-world-rspec:

1. annotated

57497 files inspected, 5785 offenses detected

2. template

57497 files inspected, 1279 offenses detected

3. unannotated

57497 files inspected, 6424 offenses detected

4. annotated + template (with AllowMultipleStyles: true and a crutch added to ConfigurableEnforcedStyle):

57497 files inspected, 320 offenses detected

The difference speaks for itself - template is by far the least offended, and the most accepted style.
Personally, I wouldn't allow multiple styles. And in general, AllowMultipleStyles don't seem to be used apart from Layout/HashAlignment.

I encourage everyone in doubt to check against a larger sample, e.g. real-world-ruby-apps.

Unless anyone objects, I intend to change the default enforced style for this cop.

Please keep in mind that:

Out of the box it will enforce many of the guidelines outlined in the community Ruby Style Guide.

A style guide that reflects real-world usage gets used, while a style guide that holds to an ideal that has been rejected by the people it is supposed to help risks not getting used at all - no matter how good it is.