-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
version: retrieve commit hash for git worktrees #2586
base: master
Are you sure you want to change the base?
Conversation
7a0ee69
to
5ffe1a7
Compare
Well it's not in
It's the same issue I described in a couple of my PRs, and is probably because you haven't reinstalled/ To me, this change is low-stakes and could be shipped in 8.0.0 while I still lack the time (😩) to work on release notes. (It's just nuts how much stuff has kept coming up for me this month! I thought I had everything planned out for $work etc. but noooo.) |
Huh, I had an old
Maybe, but let's defer it anyway IMO. |
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.
That's looking good, feature wise. Code wise, it's mostly style related.
I didn't put a comment on the f
variable, because it isn't important, however I tend to use fd
(for file descriptor) instead of just f
. In general, I prefer more expressive code and names. I don't think reducing the character count on variable names that are less than 10 characters matters.
sopel/builtins/version.py
Outdated
""" | ||
Determine the git commit hash of this Sopel, if applicable | ||
""" |
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.
""" | |
Determine the git commit hash of this Sopel, if applicable | |
""" | |
"""Determine the git commit hash of this Sopel, if applicable""" |
sopel/builtins/version.py
Outdated
""" | ||
Given paths to ``.git/`` and ``HEAD``, determine the associated commit hash | ||
""" |
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.
""" | |
Given paths to ``.git/`` and ``HEAD``, determine the associated commit hash | |
""" | |
"""Determine the associated commit hash from the git dir and its HEAD""" |
If you want, you can detail each parameter with :param gitdir: <desc>
stanza in the docstring's body.
I don't feel comfortable having this in 8.0.1. I'm sure this can wait Sopel 8.1 - beside, it should be useful mostly when working with an unstable version from git, so something that would fit nicely in master and unreleased for now. |
Agreed |
Co-authored-by: Florian Strzelecki <florian.strzelecki@gmail.com>
Co-authored-by: Florian Strzelecki <florian.strzelecki@gmail.com>
Co-authored-by: Florian Strzelecki <florian.strzelecki@gmail.com>
Co-authored-by: Florian Strzelecki <florian.strzelecki@gmail.com>
Description
Tin and personal wish fulfillment. With this changeset, the
version
command will report commit information if Sopel is running from agit-worktree
.The current revision of the
version
plugin only supports reading commit information when the host Sopel is running inside of a 'real' git repository. I find it convenient to run Sopel from a worktree for a "near to development" copy, and it's nice to still have the commit information in this use-case.Checklist
make qa
(runsmake lint
andmake test
)mypy
becausetypes-urllib3
was not installed. I'm not sure if that's missing fromdev-requirements.txt
or if my local environment was in a weird state.test_example_suggest_*
caused by content decoding errors. I only vaguely recognize the issue, but I'm confident it's not this changeset's fault.