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

Prevent warn on false positive for author/maintainer's email #116

Merged
merged 1 commit into from Mar 26, 2022

Conversation

abravalheri
Copy link
Contributor

Motivation

While I was working to support pyproject.toml metadata in setuptools, I received as a feedback from the community1 that indicates that setuptools warns the message bellow when author_email or maintainer_email are given in the form of Person Name <email@address> (and author or email are omitted):

warning: check: missing meta-data: either (author and author_email) or (maintainer and maintainer_email) should be supplied

This can be seen as a false positive, because indeed both author's name and email are being provided.
This change aims to remove this false positive result from the checks.

Notes

Please let me know if you don't think this change is suitable to this repository, and I can try to address the problem directly on the setuptools layer.

My idea on submitting it here is that it avoids the complexity of adding patches or overwriting commands in setuptools. If the users decide to use the stdlib's version of distutils, the only impact will be seeing the warning, but no feature will be compromised.

Footnotes

  1. https://discuss.python.org/t/help-testing-experimental-features-in-setuptools/13821/18

While I was working to support pyproject.toml metadata in setuptools,
I received as a feedback from the community[^1] that setuptools
warns the following message when `author_email` and `maintainer_email`
are given in the form of `Person Name <email@address>`:

> warning: check: missing meta-data: either (author and author_email)
> or (maintainer and maintainer_email) should be supplied

This can be seen as a false positive, because indeed both author's name
and email are provided.

This warning seems to happen because distutils define the `check`
command as a subcommand for `sdist`.

This change aims to remove this false positive result from the checks.

[^1]: https://discuss.python.org/t/help-testing-experimental-features-in-setuptools/13821/18
@abravalheri
Copy link
Contributor Author

abravalheri commented Feb 23, 2022

The error in the CI seems to be unrelated to the change: distutils.errors.DistutilsPlatformError: Unable to find vcvarsall.bat.

Maybe this error is also related to GitHub's update to windows-2022?

Refs:

@jaraco
Copy link
Member

jaraco commented Feb 26, 2022

I'm slightly reluctant to support two different inputs to mean the same thing. My preference would be to either:

  • accept only author/maintainer in pairs
  • accept a single parameter for author/maintainer and require it to be in the specified format to include both name and email.

(i.e. "preferably one obvious way to do it").

Actually, if I had my druthers, it would be modeled as a "contact", so maintainer: Contact and author: Contact where Contact is an object with name and email properties and which is validated to ensure both properties are met. I see that pep 621 matches my expectations.

So maybe this issue can be ignored for setup.cfg/setup.py declarations and when pep 621 support is added, it solves this issue by making name/email explicit and obviating the composite form.

Perhaps the current "false positive" behavior can be treated instead like a warning that composite values shouldn't be used, and instead, the behavior should be changed to discourage use of composite values.

Also happy to accept as-is. WDYT?

@jaraco jaraco closed this Feb 26, 2022
@jaraco jaraco reopened this Feb 26, 2022
@abravalheri
Copy link
Contributor Author

Hi @jaraco, thank you very much for the feedback.

I agree with you that it would be better to have one obvious way of doing it. (Here the core metadata spec seems to be the culprit).

Since PEP 621 explicitly require the Contact object you mentioned to be translated into author/maintainer-email, the only way of achieving a "single obvious way of doing it" would be effectively deprecating author/maintainer and enforcing the *-email fields 1.

I am more than happy to change the PR to reflect this idea. Please just give me the "go ahead" and I will proceed 😄

(However I have the impression that most of the people today use 2 separated fields)

Footnotes

  1. I am considering the metadata object here as a precursor of PKG-INFO/METADATA, instead of thinking in terms of the setup() arguments.

@jaraco
Copy link
Member

jaraco commented Mar 19, 2022

As I think about it more, I'm disinclined to add sophistication to the _email field. What we're really asking is to supply contact info (name and email) for various aspects (authorship, maintainership). Having _email in the name only confuses matters (because it's not _maybe_name_and_email). Honestly, I think the existing implementation is perfectly adequate and the warning is correct.

I looked into the reported issue, but there I see the user is supplying name and email separately. I don't see how this leads to warnings with Setuptools. The user is supplying separate name/email, setuptools expects separate name/email, and the output format expects separate name/email.

What's the motivation for encoding the name into the email?

@abravalheri
Copy link
Contributor Author

Hi @jaraco thank you very much for having a look at this.

I agree with most of the things you stated in your previous comment.
To be sincere this issue just arises once we try to use the existing setuptools/distutils machinery to ingest configuration specified according to PEP 621.

What's the motivation for encoding the name into the email?

PEP 621, is very specific about how Core Metadata should be written:

If both email and name are provided, the value goes in Author-email/Maintainer-email as appropriate, with the format {name} <{email}> (with appropriate quoting, e.g. using email.headerregistry.Address).

In summary, PEP 621 standardises the following translations:

pyproject.toml PKG-INFO
authors = [{name = "Foo Bar"}] => Author: Foo Bar
authors = [{email = "foo@bar.com"}] => Author-email: foo@bar.com
authors = [{name = "Foo Bar", email = "foo@bar.com"}] => Author-email: Foo Bar <foo@bar.com>

Since the PKG-INFO file is created from dist.metadata, I am setting the author/maintainer/author_email/maintainer_email attributes according to the translations dictated by the PEP. However, this will result in confusing warning messages for the users.

Do you have any other suggestion on how to tackle this? I don't think the changes proposed in this PR are required, I just wonder if there is another way of to not confuse the users with the warning...

@jaraco
Copy link
Member

jaraco commented Mar 26, 2022

In summary, PEP 621 standardises the following translations:

That's horrible. It requires a consumer of the PKG-INFO to have sophisticated logic to parse out the name and email of an author or authors. I guess it's a problem too of the core metadata spec.

What you've done here seems like the best thing to be done give the convoluted specs.

@jaraco jaraco merged commit 61bc807 into pypa:main Mar 26, 2022
@abravalheri abravalheri deleted the fix-check-warning branch March 26, 2022 17:07
@abravalheri
Copy link
Contributor Author

Thank you very much Jason.

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.

None yet

2 participants