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

Turn off rubocop's FormatStringToken check #61

Merged
merged 1 commit into from
Dec 18, 2017

Conversation

wfleming
Copy link
Contributor

@wfleming wfleming commented Dec 18, 2017

This appeared when we recently upgraded RuboCop: I don't think it's a
style we should enforce.

The named tokens in format strings can be useful in very complicated
cases, but those cases are not the norm, and the check's suggested style
makes simple cases less readable.

In 0.52.0 it also has bugs leading to many false positives where
changing the code as suggested would result in incorrect behavior:
rubocop/rubocop#5245. E.g. it wants me to rewrite time.strftime("%b %d, %l:%M %p") as something like time.strftime("%<month>b %<day>d, %<hour>l:%<month>M %<ampm>p"), which does not work at all.

This appeared when we recently upgraded RuboCop: I don't think it's a
style we should enforce.

The named tokens in format strings can be useful in very complicated
cases, but those cases are not the norm, and the check's suggested style
makes simple cases *less* readable.

In 0.52.0 it also has bugs leading to many false positives where
changing the code as suggested would result in incorrect behavior:
rubocop/rubocop#5245.
@wfleming wfleming requested a review from a team December 18, 2017 16:42
Copy link

@wilson wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow I didn't even know about these named tokens. If they don't work 100% the same as the short form, then I'm all for disabling this suggestion.

@wfleming wfleming merged commit 64fc34e into master Dec 18, 2017
@wfleming wfleming deleted the will/ruby-format-token branch December 18, 2017 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants