Skip to content

/cpython

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

bpo-38914 Do not require email field in setup.py. #17388

Merged
merged 4 commits into from Dec 23, 2019
Merged

bpo-38914 Do not require email field in setup.py. #17388

merged 4 commits into from Dec 23, 2019

Conversation

@jugmac00
Copy link
Contributor

jugmac00 commented Nov 26, 2019 •
edited by miss-islington

When checking setup.py and when the author field was provided, but
the author_email field was missing, erroneously a warning message was
displayed that the author_email field is required.

The specs do not require the author_emailfield:
https://packaging.python.org/specifications/core-metadata/#author

The same is valid for maintainer and maintainer_email.

The warning message has been adjusted.

modified: Doc/distutils/examples.rst
modified: Lib/distutils/command/check.py

https://bugs.python.org/issue38914

Automerge-Triggered-By: @pganssle
@jugmac00
BPO-38914 Do not require email field in setup.py.
Loading status checks…
bcb22b6 
When checking `setup.py` and when the `author` field was provided, but
the `author_email` field was missing, erroneously a warning message was
displayed that the `author_email` field is required.

The specs do not require the `author_email`field:
https://packaging.python.org/specifications/core-metadata/#author

The same is valid for `maintainer` and `maintainer_email`.

The warning message has been adjusted.

modified:   Doc/distutils/examples.rst
modified:   Lib/distutils/command/check.py
@jugmac00 jugmac00 changed the title BPO-38914 Do not require email field in setup.py. bpo-38914 Do not require email field in setup.py. Nov 26, 2019
@jugmac00 jugmac00 mentioned this pull request Nov 26, 2019
Closed
@merwok
merwok requested changes Nov 26, 2019
View changes
Lib/distutils/command/check.py Show resolved Hide resolved
Lib/distutils/command/check.py Show resolved Hide resolved
Lib/distutils/command/check.py Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Nov 26, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
@jugmac00 jugmac00 requested a review from merwok Nov 26, 2019
@jugmac00

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jugmac00 commented Nov 26, 2019

I do not know whether this matters, but @pganssle proposed a change of wording over at
pypa/pep517#73 (comment)

Presumably we can solve it by adjusting the wording, since I see no such requirement in the specification.
@jugmac00

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jugmac00 commented Nov 26, 2019

Well technically I did not
"I have made the requested changes; please review again"
as I do not think the requested changes are valid.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Nov 26, 2019

Thanks for making the requested changes!

@merwok: please review the changes made to this pull request.
@pganssle
pganssle reviewed Nov 26, 2019
View changes
Lib/distutils/command/check.py Outdated Show resolved Hide resolved
@pganssle

This comment has been minimized.

Sign in to view
Copy link
Member

pganssle commented Nov 26, 2019

@jugmac00 I am fine with the wording changes, but I do think we should put in a news entry, since this is a user-facing behavior.

Thank you for investigating this and making a PR!
@merwok
merwok approved these changes Nov 26, 2019
View changes
jugmac00 added 2 commits Nov 26, 2019
@jugmac00
Add requested news entry.
30a5f42 
new file:   Misc/NEWS.d/next/Library/2019-11-26-23-21-56.bpo-38914.8l-g-T.rst
@jugmac00
Update docstring as requested.
Loading status checks…
6f2c855 
modified:   Lib/distutils/command/check.py
@jugmac00

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jugmac00 commented Nov 26, 2019

Thank you very much for your suggestions and for leading me through this pull request.

As I do not know whether you squash pull requests anyway or you'd prefer me to do it locally and force push, I just did created some more commits for the requested changes.

I will happily squash my commits locally and force push them - just leave me a note.
@jugmac00 jugmac00 requested a review from pganssle Nov 26, 2019
@merwok

This comment has been minimized.

Sign in to view
Copy link
Contributor

merwok commented Nov 26, 2019

Squashing does not play well with github reviews and links, so CPython uses messy branches + squash merge. All info in the devguide: https://devguide.python.org/gitbootcamp/#creating-a-pull-request
@pganssle
pganssle approved these changes Dec 8, 2019
View changes
@pganssle
Adjusted wording of news entry
Unverified
The key that signed this is expired.
GPG key ID: CD54FCE3D964BEFB Learn about signing commits
Loading status checks…
f31dc94
@pganssle pganssle removed the DO-NOT-MERGE label Dec 8, 2019
@pganssle

This comment has been minimized.

Sign in to view
Copy link
Member

pganssle commented Dec 8, 2019

I pushed a change to the wording of the news entry to clarify that this is just an adjustment to the wording of the warning and that otherwise the behavior has not changed. @jugmac00 If you agree with my new wording, I am happy to merge this.
@jugmac00

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jugmac00 commented Dec 8, 2019

@pganssle Thanks for taking your time! lgtm :-)
@miss-islington miss-islington merged commit 9f9dac0 into python:master Dec 23, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191208.18 succeeded
Details
bedevere/issue-number Issue number 38914 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@merwok

This comment has been minimized.

Sign in to view
Copy link
Contributor

merwok commented Dec 23, 2019

Small detail for future changes: there is no need to list modified files in the PR description, we can see that in github tabs and git metadata 🙂
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merwok merwok

@pganssle pganssle

Assignees
No one assigned
Labels
🤖 automerge CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@jugmac00 @bedevere-bot @pganssle @merwok @the-knights-who-say-ni @miss-islington
You can’t perform that action at this time.