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

pass ScmVersion to dump_version to provide scm_version as extra template variable #854

Merged
merged 12 commits into from
May 31, 2023

Conversation

ZhiyuanChen
Copy link
Contributor

@ZhiyuanChen ZhiyuanChen commented May 25, 2023

fixes #853

related to #818
related to #577

Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
parsed_version,
version_scheme=config.version_scheme,
local_scheme=config.local_scheme,
)
if config.write_to is not None:
dump_version(
root=config.root,
version=version_string,
version=parsed_version,
Copy link
Contributor

Choose a reason for hiding this comment

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

i recommend using the name scm_version or parsed_version as input names so it can be distinguished

Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

thanks for preparing this

unfortunately the argument rename is a breaking change that will get downstreams by surprise

as such the old name needs to stay and the new object needs to go in by a different name

there may be need for extra considerations wrt what attributes to put on ScmVersion to help formatting

src/setuptools_scm/__init__.py Outdated Show resolved Hide resolved
@RonnyPfannschmidt RonnyPfannschmidt changed the title parse Version to dump_version pass ScmVersion to dump_version to provide scm_version as extra template variable May 26, 2023
Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

this looks much better now, thanks!

for completeness i would recommend to add a unit-test that uses a custom template to dump the node in addition to the normal version and checks for it

with the addition of a changelog note on the feature addition this would be ready to merge then

i presume the pdb calls will go away then

👍

Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
ZhiyuanChen and others added 2 commits May 26, 2023 16:51
Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

I took note that adding the parameter as is, does a avoidable breaking change

I recommend changing it to a optional keyword only parameter

As for testing,there is need for a own test that uses a custom template that's actually using data from the SCM version

dump_version(tmp_path, "1.0.1+g4ac9d2c", "second.py")
version = "1.0.1+g4ac9d2c"
scm_version = meta("1.0.1", node="g4ac9d2c", config=c)
dump_version(tmp_path, version, scm_version, "second.py")
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this doesn't Test The new feature

Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Excellent work, thanks

Please confirm if i should squash or merge

I'll give it a last look over once I return to the computer

Signed-off-by: Zhiyuan Chen <chenzhiyuan@dp.tech>
@ZhiyuanChen
Copy link
Contributor Author

Excellent work, thanks

Please confirm if i should squash or merge

I'll give it a last look over once I return to the computer

Thanks~ I believe it's better to squash the changes, as most commits are redundant"

@RonnyPfannschmidt
Copy link
Contributor

Thanks for enabling this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parse SCMVersion to dump_version
2 participants