Skip to content

Conversation

@di
Copy link
Member

@di di commented Apr 2, 2020

This PR ditches the ClassifierNode/ClassifierTree entirely (along with the code generation) since we don't actually need the tree hierarchy for PyPI.

Tests now run against the same classifiers/deprecated_classifiers objects that end up in the package.

Also, switches to CalVer.

@di di requested a review from ewdurbin April 2, 2020 05:07
@ewdurbin
Copy link
Member

ewdurbin commented Apr 2, 2020

This is rad! I'm a big fan of the simplification, but have a couple nits:

  • Can the ordering of the items trove_classifiers/__init__.py be enforced via test? I don't think it is crucial... would just help keep things sane. I'd say this is priority 1/10
  • The structure of deprecated_classifiers is pretty vague. Now that it's not machine generated a comment describing it would probably get good mileage. priority 5/10, worth changing but not a blocker by any means.

@di
Copy link
Member Author

di commented Apr 2, 2020

Can the ordering of the items trove_classifiers/__init__.py be enforced via test?

If you have any idea how to do this, I'm all ears. But since these are sets/dicts respectively, it'll be hard to do w/o doing some static analysis of the file.

Maybe there's a flake8 plugin or something that enforces order for elements in unordered data structures?

@di di requested a review from ewdurbin April 2, 2020 05:27
@pradyunsg
Copy link
Member

I mean... we can add a pytest test that reads the file, and checks if the set's contents are indeed correctly. It's OK to be slightly fragile at times.

PS: I'm only slightly crazy. :)

@di
Copy link
Member Author

di commented Apr 2, 2020

Created a new issue to capture this: #7.

@di di merged commit c2a8cc8 into master Apr 3, 2020
@di di deleted the simplify branch April 3, 2020 01:33
di added a commit that referenced this pull request Apr 3, 2020
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.

4 participants