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

Bump rubocop-jekyll to 0.13.0 and fix rubocop issues #252

Merged

Conversation

dunkmann00
Copy link
Contributor

I'm opening this PR in the hopes of helping move along a new release of github-metadata.

This has the same version bump of rubocop-jekyll as #251 but I also went ahead and let rubocop autocorrect the formatting issues it found.

@dunkmann00
Copy link
Contributor Author

Just curious, why is appveyor used for CI? It seems like it has something to do with testing for Windows, but GitHub Actions builds for Windows also?

@ashmaroli
Copy link
Member

ashmaroli commented Mar 1, 2023

Hello @dunkmann00,

Thank you for submitting this PR. However, I am out of context here. That means the title + description of this pull request is inadequate. Anyone stumbling across this pull request (including maintainers, at a future date) is going to scratch their heads wondering about the purpose / aim of this PR. (No, assuming context from included commit messages isn't the right way.)

On the contrary to suggested fixes from RuboCop, resorting to ENV.fetch isn't ideal here because the changed lines here all default to nil which is already the implicit result of these lines prior to being changed. If you were to follow the link(s) given in the CI report for PR #251, you shall understand the reason why RuboCop chose to suggest change(s). Regardless, I'd like you to revert use of ENV.fetch and explicitly disable the cop in .rubocop.yml. That said, it's always better to benchmark the two situations for clarity first.

The reasons why AppVeyor is still around when GH Actions exist:

  • Notice how AppVeyor has just one commit check status as against the many for GHA jobs? Notice how it's now slightly inconvenient when wishing to access reports for just Ubuntu-based tests (Hint: visual noise).
  • Perhaps, the above point is just my problem, i.e. subjective. But, let us actually compare two passing test reports for a given Ruby version, say Ruby 2.7. Notice (expand step Run script/cibuild) how while the Ubuntu-based report reads as expected, the Windows-based test hasn't actually run the underlying test-suite?!
  • Notice how the passing check for Windows-based tests led to the false assumption that the GH Action workflow was set up correctly and has run without errors, whereas AppVeyor has attempted to run the test-suite but failed due to version mismatch in appveyor.yml.

@dunkmann00
Copy link
Contributor Author

Hello @ashmaroli! My apologies on the title. I had used 'Checkout with GitHub Desktop' to get dependabot's commit and in the process of doing that I never really gave the title any attention (the current title was just auto generated). I'll fix that.

I did look at Rubocop's explanation, the change to ENV.fetch seemed unnecessary but I figured I'd let it make the auto change and then go back and correct anything as requested.

Notice (expand step Run script/cibuild) how while the Ubuntu-based report reads as expected, the Windows-based test hasn't actually run the underlying test-suite?!

😆 I didn't realize it didn't actually test anything! Well that makes perfect sense then. Would you like me to change anything in appveyor.yml to run Ruby 2.7 instead 2.6?

@dunkmann00 dunkmann00 changed the title Dependabot/bundler/rubocop jekyll tw 0.13.0 Bump rubocop-jekyll to 0.13.0 and fix rubocop issues Mar 1, 2023
@ashmaroli
Copy link
Member

Would you like me to change anything in appveyor.yml to run Ruby 2.7 instead 2.6?

@dunkmann00 Actually, getting AppVeyor to run Ruby 2.7 is a bit tricky. I'll handle it via a separate PR and will ping you after its merged.

@ashmaroli
Copy link
Member

@dunkmann00 AppVeyor has been reconfigured on the main branch. You may incorporate those changes into the branch here by running the following in your local terminal:

git pull upstream main   # 'upstream' refers to the base repo in Jekyll org. 

dependabot bot and others added 3 commits March 1, 2023 13:37
Updates the requirements on [rubocop-jekyll](https://github.com/jekyll/rubocop-jekyll) to permit the latest version.
- [Release notes](https://github.com/jekyll/rubocop-jekyll/releases)
- [Changelog](https://github.com/jekyll/rubocop-jekyll/blob/master/History.markdown)
- [Commits](jekyll/rubocop-jekyll@v0.12.0...v0.13.0)

---
updated-dependencies:
- dependency-name: rubocop-jekyll
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
@dunkmann00 dunkmann00 force-pushed the dependabot/bundler/rubocop-jekyll-tw-0.13.0 branch from 4bd70ed to 8a87c15 Compare March 1, 2023 18:50
@dunkmann00
Copy link
Contributor Author

@ashmaroli Okay I incorporated your changes and updated the other rubocop issues. Let me know if you are okay with the current changes or if you would like any of them reverted.

@ashmaroli
Copy link
Member

Current changes look fine to me. However, there's a warning from RuboCop about incorrect cop namespace in .rubocop.yml. The current PR is the ideal place to fix the cop name. Please rename and move directive to appropriate location in .rubocop.yml (alphabetical order)

Also update .rubocop.yml to ensure cops are in alphabetical order.
@dunkmann00
Copy link
Contributor Author

Should be all set now.

@ashmaroli
Copy link
Member

Thanks @dunkmann00
@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit e8abde0 into jekyll:main Mar 2, 2023
jekyllbot added a commit that referenced this pull request Mar 2, 2023
@dunkmann00
Copy link
Contributor Author

Thanks @ashmaroli! Any idea when you'll be able to release a new version?

@ashmaroli
Copy link
Member

I'm not really sure on the "when", @dunkmann00.., hopefully soon.

@jekyll jekyll locked and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants