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

Add support for author and maintainer fields #45

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

diazona
Copy link
Owner

@diazona diazona commented Aug 5, 2023

This pull request adds support for the author and maintainer fields in the plugin, and adds a bunch of tests to verify that it's working properly.

I made some arbitrary (but sensible) choices about how to handle unusual cases, like names without email addresses or vice versa, or extra commas - basically it just substitutes in an empty string any time something would be missing. I don't know if that aligns with how other tools work, but if we need to change that behavior later, it'll be easy to do so.

Closes #28

@diazona diazona added enhancement New feature or request setuptools-fields Fields in the pyproject data structure that this project needs to support labels Aug 5, 2023
@diazona diazona added this to the Initial release milestone Aug 5, 2023
@diazona diazona requested a review from sjlongland August 5, 2023 09:15
@diazona diazona removed the request for review from sjlongland August 5, 2023 09:16
@diazona diazona assigned diazona and unassigned sjlongland Aug 5, 2023
@diazona
Copy link
Owner Author

diazona commented Aug 5, 2023

I'll have to come back to this and figure out why it's breaking on 3.6

@sjlongland
Copy link
Collaborator

Ahh of course, I'm jumping the gun reviewing this with Python 3.6 being recalcitrant … but I can give it another quick look afterwards.

@sjlongland
Copy link
Collaborator

Ahh… spotted what is wrong… change in behaviour between Python 3.10 and later.

Dummy project for testing:

RC=0 stuartl@rikishi /tmp/mockpkg $ tail -v -n +0 *
==> setup.py <==
from setuptools import setup

setup(name="mockpkg", version="1.2.3")

In Python 3.6:

$ docker run --rm -ti -w /tmp/test -v $PWD:/tmp/test:ro python:3.6-alpine ash
/tmp/test # python3 setup.py --author
UNKNOWN
/tmp/test # python3 setup.py --author-email
UNKNOWN

Python 3.9:

$ docker run --rm -ti -w /tmp/test -v $PWD:/tmp/test:ro python:3.9-alpine ash
/tmp/test # python3 setup.py --author
UNKNOWN
/tmp/test # python3 setup.py --author-email
UNKNOWN

Python 3.10:

$ docker run --rm -ti -w /tmp/test -v $PWD:/tmp/test:ro python:3.10-alpine ash
/tmp/test # python3 setup.py --author
None
/tmp/test # python3 setup.py --author-email
None

You might need to check for the name/email being "UNKNOWN" as well as it being falsey.

@diazona
Copy link
Owner Author

diazona commented Aug 6, 2023

I went dependency-hunting and found that the change in behavior with respect to UNKNOWN comes from PR 138 in distutils, which was incorporated into setuptools in PR 3311, which landed in version 62.2.0. We should make sure to test with a version of setuptools older than that, but we do already have one such test configured in CI so I think we're set on that front.

Python 3.6 only supports up to setuptools 59.6.0, which explains why the tests were failing on 3.6.

@diazona diazona marked this pull request as draft August 6, 2023 07:02
@diazona diazona force-pushed the authors-and-maintainers/1/dev branch from f80e21c to ab423c2 Compare August 7, 2023 00:52
@diazona diazona marked this pull request as ready for review August 7, 2023 00:54
diazona added 4 commits August 6, 2023 18:03
This implementation assumes that the author, author_email, maintainer,
and maintainer_email strings are comma-separated lists of authors or
maintainers respectively. A number of projects (including this one) use
that method to list multiple authors/maintainers (as per
e.g. https://stackoverflow.com/a/10005265), and for projects that list
only a single person in the field, this won't hurt. We can expand this
logic in the future to support other methods of specifying multiple
authors/maintainers if necessary, perhaps even to the point of adding
a command-line option to inform the plugin how to parse those fields.
These tests verify that WritePyproject._transform_contributors() does
something reasonable in every case I could think of.
These tests run various combinations of author and maintainer metadata
through the plugin. The tests only verify the "typical" cases; I'm
leaving it to the unit tests for _transform_contributors() to ensure
that weird cases are handled in the proper way.

I made a judgment call to treat any missing metadata as an empty string,
but to never generate an author or maintainer entry in pyproject.toml
unless at least one of the name or email address is nonempty. This was
an arbitrary choice, at least in the sense that I didn't do any research
to determine how those cases are supposed to be handled (if there is any
standard for them). If necessary we can fix up the behavior later.

I wrote these tests to use the project fixture rather than manually
constructing Distribution objects because if I try the latter way,
it looks like setuptools_scm takes over and computes the version number
from the actual setuptools-pyproject-migration project, rather than
using the hard-coded version 0.0.1, which causes the tests to fail.
Hopefully in a future commit we can fix that.
This fixes the test which was (intentionally) broken by adding support
for the author and maintainer fields. I noticed that test_everything()
actually did not include maintainer metadata before, so I added it -
just a copy of the authors.
@diazona diazona force-pushed the authors-and-maintainers/1/dev branch from ab423c2 to 2c5cc38 Compare August 7, 2023 01:04
@diazona diazona requested a review from sjlongland August 7, 2023 01:06
@diazona
Copy link
Owner Author

diazona commented Aug 7, 2023

I think I have this ready now.

One note: I tried changing some of the tests to directly use Distribution objects like you did in #46, but they were picking up the wrong version number, so I figured I'd just go ahead with something that works, and hopefully we can fix that issue later.

@diazona
Copy link
Owner Author

diazona commented Aug 7, 2023

Oh, and just something to be aware of: I expect that this is going to have a merge conflict with #46, which one of us is going to have to fix, but it shouldn't be hard to deal with.

@diazona diazona assigned sjlongland and unassigned diazona Aug 10, 2023
@diazona
Copy link
Owner Author

diazona commented Aug 10, 2023

Whoops, I forgot to change the assignee when this was ready

@diazona diazona merged commit 08b5982 into main Aug 11, 2023
@diazona diazona deleted the authors-and-maintainers/1/dev branch August 11, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request setuptools-fields Fields in the pyproject data structure that this project needs to support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support authors and maintainers
2 participants