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

Style/FormatStringToken false positive with formatted input and template style enforced #7900

Closed
inkstak opened this issue Apr 22, 2020 · 10 comments · Fixed by #10160
Closed
Labels
bug good first issue Easy task, suitable for newcomers to the project help wanted

Comments

@inkstak
Copy link

@inkstak inkstak commented Apr 22, 2020

Expected behavior

When enforcing the template style:

Style/FormatStringToken:
  EnforcedStyle: template

and with the following code:

format('%<number>0.02f', value) }

Formatting input doesn't work with template style - source

%s style uses format style, but %{name} style doesn’t.

The cop should not report offenses or accept an new configuration option for formatted input.

I guess this is a similar issue reported in this comment

Actual behavior

It returns the offense :

Style/FormatStringToken: Prefer template tokens (like %{foo}) over annotated tokens (like %<foo>s).
  format('%<number>0.02f', value)

Steps to reproduce the problem

See examples above

RuboCop version

$ [bundle exec] rubocop -V
0.79.0
@inkstak inkstak changed the title Style/FormatStringToken false positive on formated style with template enforced style Style/FormatStringToken false positive with formated input and template style enforced Apr 22, 2020
@inkstak inkstak changed the title Style/FormatStringToken false positive with formated input and template style enforced Style/FormatStringToken false positive with formatted input and template style enforced Apr 22, 2020
@inkstak
Copy link
Author

@inkstak inkstak commented Apr 22, 2020

Sorry for the multiple edits, the issue was sent by mistake before I had time to read me again.

@stale
Copy link

@stale stale bot commented Oct 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale Issues that haven't been active in a while label Oct 22, 2020
@marcandre marcandre added bug good first issue Easy task, suitable for newcomers to the project help wanted and removed stale Issues that haven't been active in a while labels Oct 22, 2020
@marcandre
Copy link
Contributor

@marcandre marcandre commented Oct 22, 2020

If this is still valid, should definitely be fixed.

@samrjenkins
Copy link

@samrjenkins samrjenkins commented Jun 20, 2021

This behaviour is still present. I've taken a bit of a look into it and a solution isn't too challenging.

But is the proposed behaviour something Rubocop should support?

@inkstak proposes that

Style/FormatStringToken:
  EnforcedStyle: template

should not report offences for the following Ruby

format('%<number>0.02f', number: 1)
format('%{number}', number: 1) }

The current behaviour will report an offence on the first line (above) because it is not template format.

It strikes me that this is not an issue with the Rubocop rule. If config enforces template style, a developer should remain consistent in all instances. In the scenario where a developer finds a need to apply type, width, precision, or flags formatting to their strings, they ought to opt for the less restrictive style in their configuration:

Style/FormatStringToken:
  EnforcedStyle: annotated

and refactor away from any use of the template style.

@marcandre, what do you think?

@marcandre
Copy link
Contributor

@marcandre marcandre commented Jun 20, 2021

My understanding is that this cop helps to choose between two alternatives with equivalent output. The case '%<number>0.02f' having no equivalent, it should never raise an offense. I think having EnforcedStyle: template should mean prefer template when possible. I do not see a "strict" setting as being useful (and what would the auto-correction be?)

@samrjenkins
Copy link

@samrjenkins samrjenkins commented Jun 21, 2021

@marcandre I see your point in enforcing a style only in instances when there are alternatives. Then again, to me, mixing the use of template and annotated within the same project does feel like a bit of a code smell though. If a developer finds themselves making use of '%<number>0.02f', would it not be best practice to consistently use the annotated style throughout the project?

@marcandre
Copy link
Contributor

@marcandre marcandre commented Jun 22, 2021

@samrjenkins: Some people will feel like you, and they have a setting for this. Others will feel like me that "%{expr}" is the natural, default, simplest and clearest way and prefer using that whenever possible. In the rare case where that's not sufficient, then we need to be able to use the other form. This has the advantage of highlighting the difference between a straight conversion to string and a more complex one.

Similarly, some people prefer using double quotes everywhere. Others prefer that single quoted strings indicate a string without interpolations and double quotes are reserved for strings requiring interpolation.

@samrjenkins
Copy link

@samrjenkins samrjenkins commented Jun 22, 2021

@marcandre, I too had thought about the quotes analogy. I think it is a good one 👍 Happy to roll with your suggested behaviour. Thanks for talking this over with me.

To confirm, is the suggestion to never report an offence for strings of the form '%<number>0.02f'?

@marcandre
Copy link
Contributor

@marcandre marcandre commented Jun 22, 2021

To confirm, is the suggestion to never report an offence for strings of the form '%<number>0.02f'?

Yes

FnControlOption added a commit to FnControlRuby/rubocop that referenced this issue Oct 2, 2021
…formatted input and `template` style enforced, and add autocorrection
FnControlOption added a commit to FnControlRuby/rubocop that referenced this issue Oct 3, 2021
…formatted input and `template` style enforced, and add autocorrection
@KessaPassa
Copy link

@KessaPassa KessaPassa commented Jun 22, 2022

@marcandre This PR(#10160) looks good for me.
Is there any blocker?

bbatsov pushed a commit that referenced this issue Jun 23, 2022
…ed input and `template` style enforced, and add autocorrection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Easy task, suitable for newcomers to the project help wanted
Projects
None yet
4 participants