version: retrieve commit hash for git worktrees #2586
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
SnoopJ
force-pushed
the
feature/worktree-version-support
branch
from
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.
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.
| return result | ||
|
|
||
|
|
||
| def _resolve_git_dirs(pth: str) -> tuple[str, str]: |
There was a problem hiding this comment.
I don't like pth as a short version for path. Stripping one letter doesn't feel useful. To me, pth can be used as a suffix, or a temporary variable, but when it comes to function arguments I'd rather see them as just path.
| """ | ||
| Resolve a ``.git`` path to its 'true' ``.git/`` and `HEAD` |
There was a problem hiding this comment.
In Sopel we don't use this style, we use this one:
| """ | |
| Resolve a ``.git`` path to its 'true' ``.git/`` and `HEAD` | |
| """Resolve a ``.git`` path to its 'true' ``.git/`` and `HEAD` |
| gitdir, head = _resolve_git_dirs(GIT_DIR) | ||
| return _read_commit(gitdir, head) |
There was a problem hiding this comment.
I feel like it's a good time to add a try/except block either here or in the command callable, since this relies on I/O operations that can fail.
| """ | ||
| Determine the git commit hash of this Sopel, if applicable | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Determine the git commit hash of this Sopel, if applicable | |
| """ | |
| """Determine the git commit hash of this Sopel, if applicable""" |
| """ | ||
| Given paths to ``.git/`` and ``HEAD``, determine the associated commit hash | ||
| """ |
There was a problem hiding this comment.
| """ | |
| 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.
Description
Tin and personal wish fulfillment. With this changeset, the
versioncommand will report commit information if Sopel is running from agit-worktree.The current revision of the
versionplugin 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 lintandmake test)mypybecausetypes-urllib3was not installed. I'm not sure if that's missing fromdev-requirements.txtor 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.