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

Getting rid of rubocop offenses #945

Open
PragTob opened this issue Nov 14, 2014 · 11 comments
Open

Getting rid of rubocop offenses #945

PragTob opened this issue Nov 14, 2014 · 11 comments

Comments

@PragTob
Copy link
Member

PragTob commented Nov 14, 2014

Rubocop is a tool that automatically checks for ruby style violations. We ran an autofix but still got a couple of them in here.

If you want, you can help by getting rid of a couple of them, which would be appreciated 👍

Here is a nice website that shows an overview: http://www.refactorcop.com/shoes/shoes4

In case we don't like some of them, we might also deactivate them so rubocop doesn't scan for them any more.

Here is a current list of the offenses we have, generated through rubocop lib/ --format offenses -D --out rubocop_offenses.txt:

177  Style/Documentation
172  Metrics/LineLength
51   Metrics/AbcSize
46   Metrics/MethodLength
23   Lint/AmbiguousOperator
20   Metrics/ParameterLists
20   Style/CaseIndentation
17   Style/GuardClause
13   Style/TrivialAccessors
12   Lint/UselessAssignment
10   Metrics/ClassLength
9    Style/AccessorMethodName
9    Style/Semicolon
7    Metrics/CyclomaticComplexity
6    Style/PredicateName
5    Style/FormatString
4    Style/GlobalVars
4    Style/IfUnlessModifier
3    Metrics/PerceivedComplexity
3    Style/EvenOdd
3    Style/RegexpLiteral
2    Lint/Eval
2    Lint/RequireParentheses
2    Lint/ShadowingOuterLocalVariable
2    Lint/UselessAccessModifier
2    Style/NestedTernaryOperator
2    Style/SelfAssignment
1    Lint/RescueException
1    Lint/UnderscorePrefixedVariableName
1    Style/AsciiComments
1    Style/Blocks
1    Style/CommentAnnotation
1    Style/ConstantName
1    Style/EndOfLine
1    Style/FileName
1    Style/MultilineBlockChain
1    Style/OpMethod
1    Style/SingleLineBlockParams
1    Style/UnlessElse
--
638  Total

And here is a gist with a detailed analysis of what went wrong generated through rubocop lib/ -D --out rubocop.txt rubocop gist

Some time I'll probably go and check out the linting ones and others.

PragTob added a commit that referenced this issue Nov 15, 2014
* Things like array.each &:method are ambiguous in a sense that
  a simple white space would make it be a bitwise and. Adding
  parens makes it clearer, although I don't like parens so much
* Same thing goes for splates etc.
* #945
PragTob added a commit that referenced this issue Nov 15, 2014
* Sometimes whole side effect less method calls were just
  leftovers and hence deleted
* #945
PragTob added a commit that referenced this issue Nov 15, 2014
* parens for better clarity
* avoid shadowing of variables
* useless public/private modifiers
* variable that starts with _ should not be used
* couple of harder things remaining (eval/Exception)
* #945
@zmackie
Copy link

zmackie commented Dec 2, 2014

Hey Tobias. I've forked this and am going to get started squashing these. This'll be my first open-source contrib so I'm very excited!

@jasonrclark
Copy link
Member

Hurray, thanks for the help @Zanadar!

If you didn't pull already this afternoon, highly recommend you do before you start. We just merged an epic file move, and would hate for your efforts to hit unneeded merge conflicts!

Shoes on! ✨

@PragTob
Copy link
Member Author

PragTob commented Dec 2, 2014

Hey @Zanadar that sounds amazing! Looking forward to your contribution very much :)

I just checked and your fork has the state from after the epic file move, so that is good - get squashing away! Also, feel free to do early pull requests :) We love to review and refine together!

Thanks!

@jasonrclark jasonrclark modified the milestone: 4.0.0-rc1 Dec 26, 2014
azranel pushed a commit to azranel/shoes4 that referenced this issue Feb 7, 2015
@ybur-yug
Copy link

@Zanadar Have you got a PR youre still going to submit or have you stopped working on it? I was planning on hopping onto it myself.

@PragTob
Copy link
Member Author

PragTob commented Apr 28, 2015

I don't see changes on the repo so I think it's safe to hack on it :) Also, there are enough rubocop offences for everyone :D

Tobi

@jaredmoody
Copy link
Contributor

For the cops that seem questionable, should I ask about them here inline or open new tickets for each one?

@jasonrclark
Copy link
Member

We did a basic review of the rules when we opened this and excluded the ones we clearly didn't want, but it's very possible others are questionable. I'd say go for it with separate issues/PR's for any that you've got question on. That's a great way to discuss them.

@PragTob
Copy link
Member Author

PragTob commented May 6, 2015

👍

@jaredmoody
Copy link
Contributor

jaredmoody commented Nov 4, 2016

Note for anyone wanting to contribute to this issue:

All existing violations are recorded in .rubocop-todo.yml. To get started, run rubocop and all checks should pass. Then remove the cop you would like to fix from .rubocop-todo.yml. After fixing all violations, rubocop checks should all pass once again.

@submitnine
Copy link

Hi, even the Hacktoberfest label is from the last year, it should be valid to fix some rubocop offenes, right? :)
May my tiny PR can help a little bit. #1568 .

@jasonrclark
Copy link
Member

Totally still valid! Thanks @submitnine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants