-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Git: refactor methods and expand tests #21476
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the Git livecheck strategy to improve code organization, test coverage, and consistency with other strategies. The refactoring separates concerns by creating dedicated methods for parsing tags and extracting versions from content, and adds support for cached content via a provided_content parameter.
Changes:
- Renamed
tag_infotols_remote_tagsand simplified it to only return stdout/stderr content - Created new
tags_from_contentmethod to parse tags from git ls-remote output - Renamed
versions_from_tagstoversions_from_contentand refactored it to work with raw content instead of pre-parsed tags - Added
provided_contentparameter tofind_versionsto support content caching - Expanded test coverage to 100% for lines and branches
- Improved
preprocess_urllogic by changingpath.nil?check topath.blank?for better edge case handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Library/Homebrew/livecheck/strategy/git.rb | Refactored Git strategy methods to separate concerns, added TAG_REGEX constant, improved error handling, and aligned with Strategic interface |
| Library/Homebrew/test/livecheck/strategy/git_spec.rb | Expanded test suite with comprehensive coverage for all methods, edge cases, and error conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2237e09 to
e800ad1
Compare
|
The latest push includes test changes that address Copilot feedback:
|
This refactors some of the livecheck `Git` strategy's methods to separate concerns and improve test coverage. Namely, this: * Renames `#tag_info` to `#ls_remote_tags` and simplifies it to only return the `stdout` and/or `stderr` content in a hash. * Creates a new `#tags_from_content` method that contains some simple code to parse tags from `git ls-remote --tags` output using the `TAG_REGEX`. This code could be inlined but it may be useful to offer this as a method, in case it's ever needed outside the strategy (e.g., a `strategy` block). This also adds a `provided_content` parameter to `Git#find_versions`, to bring it in line with other strategies. I will be doing this for the rest of the strategies (and renaming the parameter to `content`) in an upcoming PR but I've isolated the changes for `Git` because they're more involved. Along with the above changes, the expanded tests bring the `Git` strategy to 100% coverage for lines and branches.
e800ad1 to
de1f553
Compare
brew lgtm(style, typechecking and tests) with your changes locally?This refactors some of the livecheck
Gitstrategy's methods to separate concerns and improve test coverage. Namely, this:#tag_infoto#ls_remote_tagsand simplifies it to only return thestdoutand/orstderrcontent in a hash.#tags_from_contentmethod that contains some simple code to parse tags fromgit ls-remote --tagsoutput using theTAG_REGEX. This code could be inlined but it may be useful to offer this as a method, in case it's ever needed outside the strategy (e.g., astrategyblock).This also adds a
provided_contentparameter toGit#find_versions, to bring it in line with other strategies. I will be doing this for the rest of the strategies (and renaming the parameter tocontent) in an upcoming PR but I've isolated the changes forGitbecause they're more involved.Along with the above changes, the expanded tests bring the
Gitstrategy to 100% coverage for lines and branches, which is the actual goal here. Thecontentparameter will eventually be used to add caching to livecheck for URLs that are fetched more than once in a given run and this is part of the foundation for that.