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
utils: add git_repository
#10245
utils: add git_repository
#10245
Conversation
Review period will end on 2021-01-08 at 05:42:47 UTC. |
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.
Thanks @SeekingMeaning!
sig { returns(T.nilable(String)) } | ||
def git_short_head | ||
sig { params(length: T.nilable(Integer)).returns(T.nilable(String)) } | ||
def git_short_head(length: 4) | ||
return unless git? && Utils::Git.available? |
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.
I know you didn't write this but: if this could be refactored to not be unless .. &&
and instead an if
I'd be grateful 🙇🏻. For bonus points in another PR: I'd love a rubocop
that checks for unless .. &&
and unless ... ||
. rubocop/rubocop#5400 was a first attempt for this in RuboCop itself but I think it'd be fine for this to just be a local cop for Homebrew until/if you're able to upstream it.
@@ -20,6 +20,7 @@ Style/Documentation: | |||
- 'utils.rb' | |||
- 'utils/fork.rb' | |||
- 'utils/gems.rb' | |||
- 'utils/git_repository.rb' |
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.
Can docs be added (as these are public methods) instead of this being added to todo
?
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.
How do I fix this style error?
Library/Homebrew/utils/git_repository.rb:4:1: C: Missing top-level module documentation comment.
module Utils
^^^^^^
What about utils/popen
and utils/fork
(which both extend Utils
)? Would adding top-level documentation comments cause conflicts?
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.
How do I fix this style error?
Ah, yes, I see. Would need to perhaps define that module in utils.rb
and document it? I think it's fine to punt on that, then.
d1015cb
to
dea4eb5
Compare
Review period ended. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?(Supersedes #10149)
This PR adds
utils/git_repository.rb
which provides the publicly available methodsUtils.git_head
andUtils.git_head_short
that can be used in formulae such ash2spec
. I added these methods separately fromutils/git.rb
so that it does not have to be excluded from theStyle/Documentation
rubocopBefore:
After:
Note:
git rev-parse --short
does not return a constant-length string—see https://stackoverflow.com/q/44286795