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

version: retrieve commit hash for git worktrees #2586

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

SnoopJ
Copy link
Contributor

@SnoopJ SnoopJ commented Dec 16, 2023

Description

Tin and personal wish fulfillment. With this changeset, the version command will report commit information if Sopel is running from a git-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

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
    • I did see some errors when running mypy because types-urllib3 was not installed. I'm not sure if that's missing from dev-requirements.txt or if my local environment was in a weird state.
    • Also saw some test failures in 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.
  • I have tested the functionality of the things this change touches

sopel/builtins/version.py Fixed Show fixed Hide fixed
@SnoopJ SnoopJ force-pushed the feature/worktree-version-support branch from 7a0ee69 to 5ffe1a7 Compare December 16, 2023 05:29
@dgw
Copy link
Member

dgw commented Dec 19, 2023

  • I did see some errors when running mypy because types-urllib3 was not installed. I'm not sure if that's missing from dev-requirements.txt or if my local environment was in a weird state.

Well it's not in dev-requirements.txt, and CI doesn't appear to install it from anywhere, but in CI its absence doesn't cause any make lint errors. Curious. Locally, pipdeptree doesn't even identify any packages as depending on types-urllib3 either, yet it's present in my environment. Let's chase this one down if it happens to anyone else.

  • Also saw some test failures in 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.

It's the same issue I described in a couple of my PRs, and is probably because you haven't reinstalled/--upgraded the dev-requirements.txt since #2519 (if I had to guess).

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.)

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Dec 19, 2023

It's the same issue I described in a couple of my PRs, and is probably because you haven't reinstalled/--upgraded the dev-requirements.txt since #2519 (if I had to guess).

Huh, I had an old urllib3. Might be why I saw types-urllib3 weirdness too. I could have sworn I installed the requirements file, but I guess I only manually installed the stubs. Maybe I left my environment in a bad state while checking something against 7.1.9.

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.)

Maybe, but let's defer it anyway IMO.

@SnoopJ SnoopJ added this to the 8.0.1 milestone Dec 19, 2023
Copy link
Contributor

@Exirel Exirel left a 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 Show resolved Hide resolved
sopel/builtins/version.py Outdated Show resolved Hide resolved
sopel/builtins/version.py Show resolved Hide resolved
Comment on lines 66 to 68
"""
Determine the git commit hash of this Sopel, if applicable
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""
Determine the git commit hash of this Sopel, if applicable
"""
"""Determine the git commit hash of this Sopel, if applicable"""

Comment on lines 23 to 25
"""
Given paths to ``.git/`` and ``HEAD``, determine the associated commit hash
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""
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.

@Exirel
Copy link
Contributor

Exirel commented Oct 18, 2024

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.

@Exirel Exirel modified the milestones: 8.0.1, 8.1.0 Oct 18, 2024
@SnoopJ
Copy link
Contributor Author

SnoopJ commented Oct 19, 2024

I'm sure this can wait Sopel 8.1

Agreed

SnoopJ and others added 7 commits October 19, 2024 15:44
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants