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.

Sign up for GitHub

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

Jump to bottom

Show warning message if passed string to 'Enabled' key in .rubocop.yml #7272

Merged
merged 1 commit into from Aug 19, 2019
Merged

Show warning message if passed string to 'Enabled' key in .rubocop.yml #7272

merged 1 commit into from Aug 19, 2019

Conversation

unasuke
Copy link
Contributor

@unasuke unasuke commented Aug 8, 2019 •
edited

feature

It causes unexpected cop warnings because treats as truthy value if passed neither true nor false in 'Enabled' key's value like "Enabled: disable".

motivation

Our team occurred a problem that, unexpected cop was in effect.
The cause was the wrong config value like an "Enabled: disabled".

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.

@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch from f38d9cb to 6241927 Compare August 8, 2019 10:05
@unasuke
Copy link
Contributor Author

unasuke commented Aug 8, 2019 •
edited

hmm, How do I fix the Metrics/ClassLength: Class has too many lines. [179/174] ...?

@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch 2 times, most recently from 5492f85 to d7863b9 Compare August 8, 2019 10:21
unasuke
unasuke commented Aug 8, 2019
View reviewed changes
.rubocop_todo.yml Outdated
@@ -17,7 +17,7 @@ Metrics/AbcSize:
# Offense count: 47
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 174
Max: 179
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't write this code for more short. So I changed max value in Metrics/ClassLength in .rubocop_todo.yml

koic
koic reviewed Aug 9, 2019
View reviewed changes
lib/rubocop/config_loader.rb Outdated Show resolved Hide resolved
@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch from d7863b9 to cd1403d Compare August 9, 2019 14:22
bbatsov
bbatsov reviewed Aug 10, 2019
View reviewed changes
lib/rubocop/config_loader.rb Outdated

warn Rainbow(
"Cop `#{parent}` is enabled by value '#{value}' specified. " \
'It may unexpected behavior unless set true or false.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should probably raise an error instead and just say that property X of cop Y is supposed to be a boolean and it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I change to raise an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

Screenshot from 2019-08-10 20-05-44

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually instead of exception this can just be an error message with some error code. I guess the stack trace will just confuse most users.

It will also be a good idea to colour the names of the cop and the property in the error message and to mention its current value that's causing the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is this?

Screenshot from 2019-08-10 21-51-25

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's good.

@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch from cd1403d to 99c15f4 Compare August 10, 2019 14:46
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 13, 2019

You'll also have to rebase on top of the current master.

@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch from 99c15f4 to 7df13d0 Compare August 14, 2019 02:01
@unasuke
Copy link
Contributor Author

unasuke commented Aug 14, 2019

I rebased this. 🙆‍♂️

koic
koic reviewed Aug 14, 2019
View reviewed changes
lib/rubocop/config_loader.rb Outdated Show resolved Hide resolved
koic
koic reviewed Aug 14, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch from 7df13d0 to 529aaab Compare August 14, 2019 04:35
koic
koic reviewed Aug 14, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@unasuke
Show warning message if passed string to 'Enabled' key in .rubocop.yml
b1e0911 
It causes unexpected cop warnings because treats as truthy value if
passed neither true nor false in 'Enabled' key's value like
"Enabled: disable".  rubocop#7056
@unasuke unasuke force-pushed the warn_in_not_boolean_to_enabled branch from 529aaab to b1e0911 Compare August 14, 2019 06:05
koic
koic reviewed Aug 16, 2019
View reviewed changes
CHANGELOG.md
@@ -5,6 +5,7 @@
### New features

* [#7274](https://github.com/rubocop-hq/rubocop/issues/7274): Add new `Lint/SendWithMixinArgument` cop. ([@koic][])
* [#7272](https://github.com/rubocop-hq/rubocop/pull/7272): Show warning message if passed string to `Enabled`, `Safe`, `SafeAutocorrect`, and `AutoCorrect` keys in .rubocop.yml. ([@unasuke][])
Copy link
Member

Choose a reason for hiding this comment

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

Ah, This URL seems to be #7056.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoud I have to change the url to #7056 ? (But already merged it... Sorry too late.)

@koic koic mentioned this pull request Aug 16, 2019
Closed
@bbatsov bbatsov merged commit 8cfdc57 into rubocop:master Aug 19, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 19, 2019

Thanks!

@unasuke unasuke deleted the warn_in_not_boolean_to_enabled branch August 19, 2019 07:38
@touhouota touhouota mentioned this pull request Nov 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koic koic koic approved these changes

@bbatsov bbatsov bbatsov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@unasuke @bbatsov @koic