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

Fix rubocop-related issues (part 1) #75

Merged
merged 12 commits into from
Mar 23, 2023
Merged

Fix rubocop-related issues (part 1) #75

merged 12 commits into from
Mar 23, 2023

Conversation

jay7x
Copy link
Member

@jay7x jay7x commented Dec 23, 2022

This PR includes rubocop-inspired fixes. Though there was few unrelated changes:

  • Gemfile.local was removed
  • vendor directory was added to the .gitignore
  • .editorconfig added (mostly to keep trailing spaces and newlines under control)
  • bin/beaker-docker was fixed (it seems it was never really working 🤔)
  • 00_default_spec.rb was rewritten in Beaker DSL (beaker-rspec was not really used there)

@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (a0398f0) 85.61% compared to head (6716908) 85.61%.

❗ Current head 6716908 differs from pull request most recent head 08189b8. Consider uploading reports for the commit 08189b8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   85.61%   85.61%           
=======================================
  Files           1        1           
  Lines         292      292           
=======================================
  Hits          250      250           
  Misses         42       42           
Impacted Files Coverage Δ
lib/beaker/hypervisor/docker.rb 85.61% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Rakefile Outdated Show resolved Hide resolved
beaker-docker.gemspec Outdated Show resolved Hide resolved
@jay7x
Copy link
Member Author

jay7x commented Dec 29, 2022

🆙

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I've been working on a script to fix it per cop. For voxpupuli/beaker#1761 I used https://gist.github.com/ekohl/b41006003553dce67b9c551f306f8080 to fix TODOs. Then in rebases I'd order things around again. Some of the commits are manual, in case there was no autofix. It relies on https://pypi.org/project/yq/ to get all the keys from the TODO file.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
beaker-docker.gemspec Show resolved Hide resolved
@jay7x
Copy link
Member Author

jay7x commented Dec 30, 2022

I've been working on a script to fix it per cop. For voxpupuli/beaker#1761 I used https://gist.github.com/ekohl/b41006003553dce67b9c551f306f8080 to fix TODOs. Then in rebases I'd order things around again. Some of the commits are manual, in case there was no autofix. It relies on https://pypi.org/project/yq/ to get all the keys from the TODO file.

Thank you for the idea! JFYI I implemented it as a rake task here! Execute it as bundle exec rake rubocop:fix_todos

@jay7x
Copy link
Member Author

jay7x commented Jan 13, 2023

Anything more is expected from me here? Or we can squash it a bit and merge?

@ekohl
Copy link
Member

ekohl commented Jan 13, 2023

I've been very busy but I'll try to revisit it.

@bastelfreak
Copy link
Member

@jay7x can you maybe rebase this please?

@jay7x
Copy link
Member Author

jay7x commented Mar 11, 2023

@jay7x can you maybe rebase this please?

Sure.. Though I think we should recheck the rubocop rules and maybe sync the config with other gems. I'll poke you later today in Slack.

Rakefile Outdated
system('git', 'commit', '-m', msg, *args)
end

namespace :rubocop do
Copy link
Member

Choose a reason for hiding this comment

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

I'm not happy with a huge implementation of this in a Rakefile. If we decide to go this route it belongs in a separate gem IMHO. Otherwise we end up duplicating it many times.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this. It's only useful when fixing the todo file, and that's done after some point. I think it should be replaced with our default block:

begin
  require 'rubocop/rake_task'
rescue LoadError
  # RuboCop is an optional group
else
  RuboCop::RakeTask.new(:rubocop) do |task|
    # These make the rubocop experience maybe slightly less terrible
    task.options = ['--display-cop-names', '--display-style-guide', '--extra-details']
    # Use Rubocop's Github Actions formatter if possible
    if ENV['GITHUB_ACTIONS'] == 'true'
      task.formatters << 'github'
    end
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

maybe task.options = ['--display-cop-names', '--display-style-guide', '--extra-details'] isn't even required anymore

beaker-docker.gemspec Outdated Show resolved Hide resolved
beaker-docker.gemspec Outdated Show resolved Hide resolved
@jay7x
Copy link
Member Author

jay7x commented Mar 17, 2023

Rakefile and GHA config should be synced across all beaker plugins I'd say.. beaker-hcloud has the simplest one which we can reuse maybe 🤔

@bastelfreak
Copy link
Member

I want to sync them, but before we do that I want to:

  • bump all plugins to Ruby 2.7
  • Introduce rubocop
  • fix most rubocop violations
  • have a shared rubocop config in beaker that's used by all beaker plugins

@bastelfreak
Copy link
Member

Edit: I'm tracking all the rubocop/ruby refactoring in https://github.com/orgs/voxpupuli/projects/4

@ekohl
Copy link
Member

ekohl commented Mar 17, 2023

@jay7x I introduced merge conflicts, but I'd also raise another point: the Rakefile has large amount of code for yard, but we don't even include it in our Gemfile. When I manually add it and run it, it fails to generate. Right now I'd suggest to drop it altogether. #96 will remove more merge conflicts.

jay7x and others added 12 commits March 18, 2023 11:03
.. and the *.gemspec because if was affected by style changes
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
`beaker-rspec` is not really used in the acceptance test. So the gem is
removed and the test is rewritten to be true Beaker DSL test.
That script was never working it seems..
Fix rubocop issues as well.
* Style/IfUnlessModifier cop
* Style/StringLiterals cop
* Metrics cops
@bastelfreak bastelfreak merged commit 2f6f44d into voxpupuli:master Mar 23, 2023
@jay7x jay7x deleted the rubocop_fixes branch March 23, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants