-
-
Notifications
You must be signed in to change notification settings - Fork 223
Add commit_id to dump_file template #1154
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
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.
i like the idea - please fix the tests and propose a change-log entry (i'll add it to the change-log)
Fixed the tests. I also renamed the new attributes to Proposed change-log entry: |
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 unfortunately it seems like that one test missed in naming
Perhaps i should have given the template in the test a more detailed name
I also took note that there's potential situations where nodeid isn't known
I believe those where the reason for not including it in the beginning
But I'm unaware of a easy acceptance test for that so i need to log a follow-up
"__version__ = version = '1.0.dev42'", | ||
"__version_tuple__ = version_tuple = (1, 0, 'dev42')", | ||
"", |
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.
This should get a different test
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 agree, I added a second test that doesn't use the test template but where the commit id is not None
testing/test_basic_api.py
Outdated
@@ -23,7 +23,7 @@ | |||
template = """\ | |||
__version__ = version = {version!r} | |||
__version_tuple__ = version_tuple = {version_tuple!r} | |||
__sha__ = {scm_version.node!r} | |||
__sha__ = sha = {scm_version.node!r} |
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.
The template is intentionally different from the normal template
If its the same using it in test no longer shows different behavior
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.
Ah ok understood, I changed it back
@@ -31,9 +31,13 @@ | |||
__version__: str | |||
__version_tuple__: VERSION_TUPLE | |||
version_tuple: VERSION_TUPLE | |||
sha: str |
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.
Lets use the name node_id or commit_id
Just sha is actually incorrect for scm like jj pujil or darcs
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.
Alright, I changed it to commit_id
for more information, see https://pre-commit.ci
I changed the type of the attribute to |
well done, i'll merge + add a changelog entry after i finish sorting out my other pr |
Alright, thank you! |
For my use case I want to report not only the current version of my application but also the git hash at which it was built. setuptools-scm includes the git hash if the version isn't exact (i.e. the application wasn't built from a commit with a git tag), but not if it is. This behavior on it's own is fine and expected imo. However I still needed an easy way to access the git hash in these cases in addition to the version, so I maid this small adjustment to the dump_file template so that it always includes the current git_hash in addition the the version and version_tuple, regardless of whether the version or version_tuple includes the git hash already or not.
I am currently already using this change for my own packages project-W and project-W-runner, but maybe they are useful to other users as well so I decided to create this PR.