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

cask/audit: allow @ for versioned casks #16865

Merged
merged 1 commit into from Mar 11, 2024
Merged

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Mar 9, 2024

  • 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?

Specifically for homebrew-cask-versions migration into homebrew-cask (Homebrew/homebrew-cask#112102).

Unlike Formulae, we don't need to worry about replacing @ in token name to AT so can handle this the easy way by just loosening audit.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

@bevanjkay
Copy link
Member

Thanks @cho-m

@cho-m
Copy link
Member Author

cho-m commented Mar 11, 2024

Going to merge this to unblock us in migration. We can always modify audit further if needed or we find this approach isn't ideal.

cc @Homebrew/cask if anyone wants to make any comments after merge, which I will follow up on.

@cho-m cho-m merged commit a30f6c4 into Homebrew:master Mar 11, 2024
26 checks passed
@cho-m cho-m deleted the cask-audit-@ branch March 11, 2024 13:07
@reitermarkus
Copy link
Member

What should happen to casks that currently use -at-, e.g. folding-at-home and folding-at-home-beta?

Is folding@home@beta fine? Do we then treat folding@home different from e.g. folding@beta.

@reitermarkus
Copy link
Member

Oh right, I missed “cask token @ unrelated to versioning …”.

@cho-m
Copy link
Member Author

cho-m commented Mar 11, 2024

Oh right, I missed “cask token @ unrelated to versioning …”.

I went with the simplest option of only allowing a single @ and that it should not be used in base/unversioned name.

One thing the audit doesn't catch is misuse of that @. We can make this stricter by only allowing version formats and a fixed list of "terms" (e.g. alpha, beta, dev, ...), but I wasn't sure what those needed to be.

For formulae, we check there is an unversioned formula. If this can be guaranteed for official Casks, then we can add audit.

@MikeMcQuaid
Copy link
Member

Thanks again @cho-m!

@reitermarkus
Copy link
Member

If this can be guaranteed for official Casks, then we can add audit.

I think so. There won't be an app@beta if there is no stable app. There won't be an app@1 if there is no app with version 2.

@p-linnane
Copy link
Member

I think so. There won't be an app@beta if there is no stable app. There won't be an app@1 if there is no app with version 2.

We do accept casks that only have unstable versions with no stable release. There may be a case for using @beta there.

@cho-m
Copy link
Member Author

cho-m commented Mar 12, 2024

I think so. There won't be an app@beta if there is no stable app. There won't be an app@1 if there is no app with version 2.

We do accept casks that only have unstable versions with no stable release. There may be a case for using @beta there.

Don't we use unversioned name for these and just keep the name when it becomes stable? At least according to our (soon outdated) docs: https://docs.brew.sh/Acceptable-Casks#but-there-is-no-stable-version. I think the current approach is fine to avoid extra versioned Casks.

If we migrate some Casks without modifying name, e.g. safari-technology-preview, then it may be fine to do audit. Or if there are edge cases, adding an audit exception for those.

@reitermarkus
Copy link
Member

reitermarkus commented Mar 12, 2024

We do accept casks that only have unstable versions with no stable release. There may be a case for using @beta there.

Yes, but we just use the unversioned name in that case, since there will eventually be a stable version.

@cho-m
Copy link
Member Author

cho-m commented Mar 12, 2024

Also, opened WIP update to audit in #16882. Although we don't need it for homebrew/cask-versions, I think it is worth adding . support in version since we support that in formulae and external Casks use this naming. And modifying the outdated -beta/etc check.

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
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