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

rubocops (cask/url): add rubocop to use csv instead of before|after_comma #12648

Merged
merged 7 commits into from Jan 12, 2022

Conversation

bevanjkay
Copy link
Member

@bevanjkay bevanjkay commented Dec 31, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This adds a rubocop to check cask URLs for version.before_comma and version.after_comma and replace them with csv.first and csv.second respectively.

We should discuss whether to start this as an offence or a warning, bump-cask-pr will automatically update any offending casks.

@bevanjkay bevanjkay requested a review from a team December 31, 2021 00:09
@BrewTestBot
Copy link
Member

Review period will end on 2022-01-03 at 00:09:08 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 31, 2021
@bevanjkay
Copy link
Member Author

I'm not sure how to get the check to skip when a block is defined for the URL. I assume that's what the issue is with the one failing test.

@MikeMcQuaid
Copy link
Member

I'm not sure how to get the check to skip when a block is defined for the URL. I assume that's what the issue is with the one failing test.

node.type == :block

Please also add some new tests for this, thanks!

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 3, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@bevanjkay
Copy link
Member Author

@MikeMcQuaid I'm struggling to come up with tests that work for this. I am assuming I need to import the version system somehow to the tests. Is there someone that can point me in the right direction?

@MikeMcQuaid
Copy link
Member

@bevanjkay Try to copy/paste existing RuboCop cask tests? @reitermarkus may be able to help more.

@bevanjkay
Copy link
Member Author

@MikeMcQuaid I did do that. But I need to be able to mimic the interpolation in the strings. That's the part that I'm getting stuck at. I can push up as far as I got to tomorrow.

bevanjkay and others added 2 commits January 11, 2022 01:22
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
Copy link
Member

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

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

Following up from on Slack, I thought I'd be nice and suggest the interpolation escaping changes here.

@bevanjkay
Copy link
Member Author

bevanjkay commented Jan 12, 2022

@MikeMcQuaid Should the style syntax fixes be merged (in the cask repos) before this PR is merged?

@issyl0
Copy link
Member

issyl0 commented Jan 12, 2022

@bevanjkay Yes, they should. Merging those is a prerequisite for the tap-syntax CI check on this PR to go green.

@bevanjkay
Copy link
Member Author

Thanks @issyl0 the PRs have been merged. Are you able to restart the CI run now?

@miccal
Copy link
Member

miccal commented Jan 12, 2022

Thanks @issyl0 the PRs have been merged. Are you able to restart the CI run now?

Restarted.

@MikeMcQuaid MikeMcQuaid merged commit 3ba6afb into Homebrew:master Jan 12, 2022
@MikeMcQuaid
Copy link
Member

Thanks again @bevanjkay, great work, thanks for sticking with it 💪🏻

@carlocab
Copy link
Member

👏

@github-actions github-actions bot added the outdated PR was locked due to age label Feb 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants