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

dev-cmd/audit: fix cask arg handling #12375

Merged
merged 4 commits into from
Nov 8, 2021
Merged

dev-cmd/audit: fix cask arg handling #12375

merged 4 commits into from
Nov 8, 2021

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Nov 3, 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 fixes an issue where brew audit --cask --new-cask did not run the check_token_conflicts check as it should have. This was because at some point in the past, args.token_conflicts? started returning true/false instead of true/nil. Since false.nil? is false but false.blank? is true, this wasn't triggering properly. There are a few other implied audit rules that likely were affected as a result of this. that I've fixed here, too.

@Rylan12 Rylan12 added the critical Critical change which should be shipped as soon as possible. label Nov 3, 2021
@BrewTestBot
Copy link
Member

BrewTestBot commented Nov 3, 2021

Review period ended.

@Rylan12 Rylan12 changed the title cask/audit: fix arg handling dev-cmd/audit: fix cask arg handling Nov 6, 2021
@Rylan12
Copy link
Member Author

Rylan12 commented Nov 6, 2021

I realized that it is valuable to have a distinction between nil and false for testing. That way, false means "this is 100% false never set it to true" and nil means "unset so make whatever changes are needed."

Now, we can set new_cask to true while keeping online off. Before, setting online to false was treated as if --online wasn't passed (because there's no --no-online flag) and so the --new was overriding the value set in the test.

Instead, we handle the change in the brew audit command, itself, by adding || nil to pass nil (i.e. "unset") to the auditor whenever args.foo? returns false.

In other words, the argument parser returns false when a (non-boolean) switch isn't set, and the auditor expects nil to mean unset. So, we change falses to nils before passing them.


--[no-]appcast is excluded from all of the above because the argument parser already returns nil/false/true because it's a boolean switch.

@Rylan12 Rylan12 merged commit 2537b8d into Homebrew:master Nov 8, 2021
@Rylan12 Rylan12 deleted the fix-audit-args branch November 8, 2021 05:47
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants