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

Check project against valid and deprecated trove classifiers. #2881

Merged
merged 1 commit into from
Sep 18, 2022
Merged

Check project against valid and deprecated trove classifiers. #2881

merged 1 commit into from
Sep 18, 2022

Conversation

kdeldycke
Copy link
Contributor

Inspects trove classifiers on check CLI command calls, and look for unrecognized and deprecated categories.

Adds dependency https://github.com/pypa/trove-classifiers, a package published and maintained by PyPA that is cataloguing all classifiers. This is the canonical source of all trove definitions.

Pull Request Check List

Resolves: #2579

  • Added tests for changed code.
  • Updated documentation for changed code.

@kdeldycke
Copy link
Contributor Author

This PR is failing on Python 2.7 because it adds a dependency on calver, which requires python >= 3.5.

I guess this PR should be tied to Poetry 1.2 release then: #2683

@abn
Copy link
Member

abn commented Sep 10, 2020

@kdeldycke unfortunately yes, if you really want it sooner you will need to disable the code for PY2 and make the dependency optional. That said, it is unlikely this will make 1.1.0 anyway.

@abn abn added kind/feature Feature requests/implementations area/pyproject Metadata/pyproject.toml-related labels Sep 10, 2020
@kdeldycke
Copy link
Contributor Author

Thanks @abn for the feedback! I'll wait for the next release then! :)

@abn abn added this to the 1.2 milestone Sep 10, 2020
@kdeldycke
Copy link
Contributor Author

I think this PR is ready to be merged, now that I rebased it on top of master, i.e. the future 1.2 release.

Unfortunately some tests are failing for what looks like something that has nothing to do with the code changes in this PR... 😞

@kdeldycke
Copy link
Contributor Author

All checks have passed, this PR is ready for review! :)

@finswimmer finswimmer requested a review from a team October 30, 2020 19:00
@kdeldycke
Copy link
Contributor Author

I just rebased this PR on top of upstream and fixed all remaining issues. All tests are passing.

This PR is ready to be merged for Poetry 1.2.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

This feature looks good overall, but I have some code-level changes I'd like to see. Additionally, I would like to see this code factored out into a helper rather than inline in the check command, and some more thought given to variable naming to help keep it maintainable later.

src/poetry/console/commands/check.py Outdated Show resolved Hide resolved
src/poetry/console/commands/check.py Outdated Show resolved Hide resolved
src/poetry/console/commands/check.py Outdated Show resolved Hide resolved
src/poetry/console/commands/check.py Outdated Show resolved Hide resolved
@kdeldycke
Copy link
Contributor Author

All concerns identified during the code review have been addressed. All tests passes. This PR is ready to be merged.

Do you want me to squash the commits into one?

This feature looks good overall, but I have some code-level changes I'd like to see. Additionally, I would like to see this code factored out into a helper rather than inline in the check command, and some more thought given to variable naming to help keep it maintainable later.

As for these structural code changes, I regrouped all the code into a naive helper in: 89eba1e

Is that what you were looking for? Do you have suggestions on the variables to rename?

@kdeldycke kdeldycke requested a review from neersighted June 24, 2022 06:57
@neersighted
Copy link
Member

Hey @kdeldycke -- sorry about the wait on this one, I never commented here, but it was concluded that this wasn't something we wanted to include in 1.2. I've moved the milestone to 1.3, and hope to have this reviewed and merged in time for the next minor release.

@neersighted neersighted modified the milestones: 1.2, 1.3 Sep 5, 2022
@neersighted neersighted enabled auto-merge (squash) September 18, 2022 03:33
@neersighted neersighted added the impact/changelog Requires a changelog entry label Sep 18, 2022
@neersighted neersighted merged commit ddf36aa into python-poetry:master Sep 18, 2022
@kdeldycke
Copy link
Contributor Author

1.3 is a perfectly fine target! Thanks @neersighted for the merge and sorry for the late reply!

@kdeldycke kdeldycke deleted the validate-trove-classifiers branch September 18, 2022 07:26
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/pyproject Metadata/pyproject.toml-related impact/changelog Requires a changelog entry kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Trove classifiers from canonical PyPA definitions
4 participants