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

Start Rubocop for existing codebase #24

Merged
merged 4 commits into from
Mar 21, 2018
Merged

Conversation

dsalahutdinov
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Mar 12, 2018

Codecov Report

Merging #24 into master will increase coverage by <.01%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage    96.7%   96.71%   +<.01%     
==========================================
  Files          38       38              
  Lines        1731     1734       +3     
==========================================
+ Hits         1674     1677       +3     
  Misses         57       57
Impacted Files Coverage Δ
lib/ossert/workers/fetch_bestgems_page.rb 100% <ø> (ø) ⬆️
lib/ossert/stats/community_total.rb 100% <ø> (ø) ⬆️
lib/ossert/workers/fork_processing.rb 70% <ø> (ø) ⬆️
lib/ossert/stats/community_quarter.rb 100% <ø> (ø) ⬆️
lib/ossert/workers/fetch.rb 100% <ø> (ø) ⬆️
lib/ossert/presenters.rb 100% <ø> (ø) ⬆️
lib/ossert/workers/refresh_fetch.rb 90.9% <ø> (ø) ⬆️
lib/ossert/workers.rb 100% <ø> (ø) ⬆️
lib/ossert.rb 100% <ø> (ø) ⬆️
lib/ossert/workers/partial_refresh_fetch.rb 100% <ø> (ø) ⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00520c7...6ee6cdb. Read the comment docs.

# Cop supports --auto-correct.
Layout/CommentIndentation:
Exclude:
- 'lib/ossert/repositories.rb'
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Why do I see there much more Exclude: then before?
Also, most of them support auto-correct option.
Maybe, it's worth to run rubocop -a and commit (and review) those changes.

@dsalahutdinov
Copy link
Contributor Author

Hi @sclinede I have done rubocop -a


# Offense count: 27
Metrics/AbcSize:
Max: 150
Copy link
Member

Choose a reason for hiding this comment

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

Was 16 but became 150... Maybe we could Exclude something here and restore previous restriction?


Style/Documentation:
Enabled: false
Max: 240
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I don't like such large lines. Maybe we could Exclude some files and enforce previous value.

Exclude:
- 'lib/ossert/fetch/github.rb'
- 'lib/ossert/fetch/stackoverflow.rb'
Copy link
Member

Choose a reason for hiding this comment

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

Only 2 cases. Maybe we're able to fix them, too

# Offense count: 1
Naming/MemoizedInstanceVariableName:
Exclude:
- 'lib/ossert/fetch/rubygems.rb'
Copy link
Member

Choose a reason for hiding this comment

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

Only 1 case. Maybe it worth to push it)

Exclude:
- 'lib/ossert/fetch/github.rb'
- 'config/sidekiq.rb'
Copy link
Member

Choose a reason for hiding this comment

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

Only 1 case

@dsalahutdinov
Copy link
Contributor Author

dsalahutdinov commented Mar 21, 2018

Finally I came back to previous limit values (that was, but did not worked 😄 ), added some excludes and fixed some minor offences
🆙

@sclinede
Copy link
Member

Perfect 👍

@sclinede sclinede merged commit 2c895c7 into ossert:master Mar 21, 2018
@dsalahutdinov
Copy link
Contributor Author

Cool. Thank you for patience 😄

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