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 dots in version and check unversioned cask #16882

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Mar 12, 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?

This just polishes up some checks. Shouldn't block attempting migration so can be handled in parallel.

unversioned_token, _, version_designation = cask.token.rpartition("@")
if unversioned_token.empty?
match_data = /-(?<designation>alpha|beta|rc|release-candidate)$/.match(cask.token)
if match_data && cask.tap&.official? && cask.tap != "homebrew/cask-versions"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if match_data && cask.tap&.official? && cask.tap != "homebrew/cask-versions"
if match_data && cask.tap&.official? && cask.tap.name != "homebrew/cask-versions"

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if we should only apply to homebrew/cask given audit failures in fonts. From original location of check (which I'll probably move code back to later), I'm guessing we either don't apply that audit or there were no new fonts with -beta|alpha.

As I recall, we have automation there that picks those names?

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely fix the automation, but maybe this should only apply to brew audit --new for now?

add_error "unversioned cask token @ should be replaced by -at-" if unversioned_token.include? "@"

if unversioned_token.match?(/[^a-z0-9-]/)
add_error "unversioned cask token should only contain lowercase alphanumeric characters and hyphens"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
add_error "unversioned cask token should only contain lowercase alphanumeric characters and hyphens"
add_error "cask token should only contain lowercase alphanumeric characters and hyphens"


return if version_designation.empty?

add_error "unversioned cask token should not have trailing hyphens" if unversioned_token.end_with?("-")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
add_error "unversioned cask token should not have trailing hyphens" if unversioned_token.end_with?("-")
add_error "cask token should not have trailing hyphens" if unversioned_token.end_with?("-")

version_designation = ""
end

add_error "unversioned cask token @ should be replaced by -at-" if unversioned_token.include? "@"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
add_error "unversioned cask token @ should be replaced by -at-" if unversioned_token.include? "@"
add_error "cask token should use '-at-' instead of '@' in the unversioned part" if unversioned_token.include? "@"

Copy link
Member Author

@cho-m cho-m Mar 12, 2024

Choose a reason for hiding this comment

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

Thanks. I just realized that "unversioned cask" was poor phrasing given its existing usage for unversioned URLs (bump-unversioned-casks).

"lowercase alphanumeric characters, hyphens and '.'"
end

if version_designation.start_with?("-", ".") || version_designation.end_with?(".")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if version_designation.start_with?("-", ".") || version_designation.end_with?(".")
if version_designation.start_with?("-", ".") || version_designation.end_with?("-", ".")

end

if version_designation.start_with?("-", ".") || version_designation.end_with?(".")
add_error "cask token version designation should not have leading or trailing hyphens and/or '.'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
add_error "cask token version designation should not have leading or trailing hyphens and/or '.'"
add_error "cask token version designation should not have leading or trailing hyphens and/or dots"


if version_designation.match?(/[^.a-z0-9-]/)
add_error "cask token version designation should only contain " \
"lowercase alphanumeric characters, hyphens and '.'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"lowercase alphanumeric characters, hyphens and '.'"
"lowercase alphanumeric characters, hyphens and dots"

Copy link

github-actions bot commented Apr 3, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Apr 3, 2024
@cho-m cho-m removed the stale No recent activity label Apr 6, 2024
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants