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

Hotfix/rubocop update #63

Merged
merged 1 commit into from Apr 21, 2020

Conversation

Dandush03
Copy link
Contributor

@Dandush03 Dandush03 commented Apr 7, 2020

Stickler update it's Rubocop Linter from Rubocop 0.79.0 to Rubocop 0.80.0. This PR updates .rubocop.yml both for ruby and RoR projects accordingly.

@MauricioRobayo
Copy link
Contributor

MauricioRobayo commented Apr 7, 2020

Hi @Dandush03 , thanks for opening this PR.

Could you please just keep on this PR the changes made to Rubocop to fix the issue with Stickler.

All other changes are great and are worth discussing them, maybe you can open an issue, but it would be easier to merge this PR if you only add the changes needed to fix the issue with Stickler after the Rubocop update.

You can force push to your branch after you remove the other changes to update this PR. So try to just have two commits, one for Ruby rubocop.yml and one for RoR rubocop.yml.

And it would be great if you can add some context on the current issue with Stickler and why this fix is needed.

Here is a previous issue related to this: #56.

Thanks for taking the time to do this!

@Dandush03
Copy link
Contributor Author

Hi @MauricioRobayo, Fallowing your suggestions

Rubocop Stickler Update

The issue, Stickler update it's Rubocop Linter from Rubocop 0.79.0 to Rubocop 0.80.0

In this new version if we want to add some linters specification we would have to enable the by default

This is something that is set to true by default whiout any file added in rubocop gem but we have to specified this now in this new version

Some other issue that I found irrelevant in this new update in Rails nor Ruby at this learning stage where the followings;

  • Copyright
  • InlineComment (It breake some rails gems comments)
  • MissingElse (This update force you to have for each if an else)
  • MethodCalledOnDoEndBlock

Let me know if it would need some extra modification 👍

Appreciate your help a lot ❗

@MauricioRobayo
Copy link
Contributor

@Dandush03 Thanks for updating the PR. One more request, could you please remove the CONTRIBUTION.md file.

Git and GitHub are going to keep track of everything.

Anyone can check the contributors and the changes they made on the contributors' tab. And the changes you made are going to be on the commit history of the repo.

So everything is already tracked, and we also have this PR as a reference.

Thanks again for working on this!

@MauricioRobayo
Copy link
Contributor

MauricioRobayo commented Apr 8, 2020

@nidalaa @gootyfer

Current issue

Rubocop v0.8 was released and new cops were added.

Every time Rubocop >=0.8 runs it is giving an informative message about those new cops (rubocop/rubocop#7731 (comment)) and Stickler is failing because of that message.

This is the message Rubocop displays locally. It's just a message, not an error neither a warning:

$ rubocop -v
0.81.0
$ rubocop
The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file:
 - Lint/RaiseException (0.81)
 - Lint/StructNewOverride (0.81)
 - Style/HashEachMethods (0.80)
 - Style/HashTransformKeys (0.80)
 - Style/HashTransformValues (0.80)
For more information: https://docs.rubocop.org/en/latest/versioning/

But because of that message Stickler is failing.

Here is a test PR with the current configuration on the first commit: https://github.com/MauricioRobayo/ruby-linters-config-test/pull/1

Stickler is failing and is not even reporting issues with the code:

image

Solution

We need to add the new cops to the current config to have Stickler working again:

Lint/RaiseException:
  Enabled: false
Lint/StructNewOverride:
  Enabled: false
Style/HashEachMethods:
  Enabled: false
Style/HashTransformKeys:
  Enabled: false
Style/HashTransformValues:
  Enabled: false

The second commit on that PR just adds the new cops (https://github.com/MauricioRobayo/ruby-linters-config-test/pull/1/commits/c508533817756b26919daaa8fae0fd9daf3d738c). After that, Stickler is working again reporting issues.

The third commit is just fixing the issues reported by Stickler to test everything is working as expected.


@Dandush03 Can you double-check that you are adding just what's needed to have Stickler running again. I can see more changes to rubocop.yml file, are those necessary?

@Dandush03
Copy link
Contributor Author

@Dandush03 Thanks for updating the PR. One more request, could you please remove the CONTRIBUTION.md file.

Git and GitHub are going to keep track of everything.

Anyone can check the contributors and the changes they made on the contributors' tab. And the changes you made are going to be on the commit history of the repo.

So everything is already tracked, and we also have this PR as a reference.

Thanks again for working on this!

Hi Again! ✋

Hope you all doing well.

I removed the CONTRIBUTION.md file as required

I did some research and as far as I could think of the Copyright, InlineComment, MissingElse, MethodCalledOnDoEndBlock will be necessary implementation for Ruby 3.0 but since the oding project specified a ruby -v 2.6.5 setup. This implementation will suit to it to avoid unnecessary Linter issue, although I think there as some other Linters error that could be set to false but since they don't make any inconsistency with Ruby version 2.6.5 like MissingElse, it will depend on Microverse criteria.

In any case if there is any modification you would want to update, I can do them right away.


PS: I gave this rubocop file to 5 student and all of then said that there Stickler is working now

Regarding my new commits

Basically I had some syntax and indent issue to fix that I found while rechecking my code 😉

ror/.rubocop.yml Outdated
@@ -1,4 +1,6 @@
AllCops:
EnabledByDefault: true
Copy link
Contributor

Choose a reason for hiding this comment

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

@MauricioRobayo @Dandush03 thank for your research on that. ❤️

According to the test repo mentioned by @MauricioRobayo we need those cops to be listed in rubocop config file:

Lint/RaiseException:
  Enabled: false
Lint/StructNewOverride:
  Enabled: false
Style/HashEachMethods:
  Enabled: false
Style/HashTransformKeys:
  Enabled: false
Style/HashTransformValues:
  Enabled: false

Can we please explicitly add those cops instead of enabling all by default and dealing with the ones that we obviously do not want to enable?

Thanks to that we have a more clear image in our Rubocop config - you can see what is enabled at first sight.
And controlling this file will be easier.

@nidalaa
Copy link
Contributor

nidalaa commented Apr 9, 2020

Thank you for all the hard work on that gentlmen! ❤️

I left only one but crucial comment. Do you want to deal with it @Dandush03 ?

@Dandush03
Copy link
Contributor Author

Hello All!

Sorry from the late response but somewhere I lost the notification regarding this PR

I deleted the EnabledByDefault: true and since we aren't enabling all by default i also deleted does integrated by me and set to false like: Copyright, InlineComment.. etc

Please let me know if more changes are required

@s0kil
Copy link

s0kil commented Apr 15, 2020

EnabledByDefault: true is actually a good default and is future proof if Robocop gets more rules in the future.
If there are ones that you want to explicitly disable you could also do that since it will be higher priority (take precedence).

@MauricioRobayo
Copy link
Contributor

MauricioRobayo commented Apr 16, 2020

@s0kil Thanks for the feedback.

Good point. Yes, it can be a good default, depending on the use case. 👍

There seems to be also a NewCops option with the default value of pending, just for the case we are addressing here. So we could also set that to enable.

That NewCops option was introduced "to solve the commonly reported problem that RuboCop updates are very painful for end users as usually they break their builds due to new cops being added" (rubocop/rubocop#5979 (comment)).

Having New cops: pending prevents new cops raising issues in the current code base. Before, new cops were enabled by default, and people complained about that (rubocop/rubocop#7731 (comment)).

So, enabling them by default could raise issues with the current code, and we might not be aware of enabling something we don't want to enable as it will silently be enabled by default. The same way, if we disable them by default to avoid raising issues with the current code base, we might never notice new cops were introduced.

In the early versions of RuboCop a common source of frustration was that new cops were added to pretty much every release, and as they were enabled by default, every upgrade resulted in broken CI builds and trying to figure out what exactly was changed. After considering many options to address this eventually we opted for an approach that limits these type of changes to major RuboCop releases.

Now new cops introduced between major versions are set to a special pending status and are not enabled by default. A warning is emitted if such cops are not explicitly enabled or disabled in the user configuration.

https://docs.rubocop.org/en/stable/versioning/#pending-cops

New cops are introduced this way so we can make a decision on what to do with them, it is a feature we might not want to be muted. 😄

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.

None yet

5 participants