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

move version into pyproject.toml #640

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

sigma67
Copy link
Contributor

@sigma67 sigma67 commented Aug 1, 2024

Description

Fixes #<!-- add the associated GitHub Issue ID>

Checklist

  • [x ] Make sure changes are covered by existing or new tests.
  • [ x] For at least one Python version, make sure test pass on your local environment.
  • [x ] Create a file in src/towncrier/newsfragments/. Briefly describe your
    changes, with information useful to end users. Your change will be included in the public release notes.
  • [x ] Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
  • [x ] Ensure docs/tutorial.rst is still up-to-date.
  • [- ] If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
  • [x ] If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

@sigma67 sigma67 requested a review from a team as a code owner August 1, 2024 08:51
@sigma67
Copy link
Contributor Author

sigma67 commented Aug 1, 2024

@adiroiban as requested this is the separate PR to move the version into pyproject.toml.

I suggest we also remove the deprecated __version__ attribute as it has been deprecated for three releases now.

@sigma67 sigma67 force-pushed the move-version-pyproject branch from 4d97dd5 to b1f92e7 Compare August 1, 2024 09:14
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks. It looks very good.

Just a comment regarding _get_metadata_version and the removal information should have a separate fragment.

Other than that, all good.

Thanks again

@@ -67,6 +69,10 @@ def get_version(package_dir: str, package: str) -> str:
if version := _get_metadata_version(package):
return version

with contextlib.suppress(PackageNotFoundError):
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this code?

What does it to, on top of the existing _get_metadata_version ?

Maybe move this to _get_metadata_version

If this needs to stay here, then it needs a comment to explain why _get_metadata_version is not good enough here and why we get the version again via `importlib.metadata.

Copy link
Contributor Author

@sigma67 sigma67 Aug 1, 2024

Choose a reason for hiding this comment

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

The test test_version_from_metadata will fail without this code.

The existing function _get_metadata_version_ uses importlib.metadata.packages_distributions, which seems to not contain towncrier at test runtime. Not sure why, but I also don't know why this method of getting the version is used instead of straight up passing package to importlib.metadata.version.

I can move the check into the function, maybe inside the if not distribution_names block. Not sure.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think that _get_metadata_version should do everything.

So the code should be moved there.

Maybe also remove importlib.metadata.packages_distributions and replace it with the current method.

Do we need both methods? Just asking.

I am not very familiar with the importlib API ... but as long as the tests pass, we should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I thought there might be some reason why it was done this way.

I replaced it as you suggested.

@@ -0,0 +1,4 @@
Moved towncrier version definition from src/towncrier/_version.py to pyproject.toml

towncrier.__version__ was removed, after being deprecated in 23.6.0.
Copy link
Member

Choose a reason for hiding this comment

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

these 2 lines needs to be in a separate newsfragment file of type .removal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@adiroiban adiroiban 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 the cleanup.


We should not worry about keeping "zombie" code.

If we think that some part of the code is not needed, we should just delete it.

As long as the tests still pass, it should be ok.

@adiroiban adiroiban merged commit fc3a255 into twisted:trunk Aug 1, 2024
16 checks passed
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.

2 participants