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

Metadata 1.3 PEP #1

Closed
wants to merge 15 commits into from
Closed

Metadata 1.3 PEP #1

wants to merge 15 commits into from

Conversation

ncoghlan
Copy link
Collaborator

@ncoghlan ncoghlan commented Dec 2, 2017

Internal PRs within a fork are a handy format for reviews prior to submitting a PR against the main repo :)

Copy link
Collaborator Author

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

I like this as a starting point. I've made a few PEP editing comments inline, and added a comment noting the other changes I'm aware of that need to be backfilled.

pep-9999.txt Outdated
Version: $Revision$
Last-Modified: $Date$
Author: Dustin Ingram <di@di.codes>
Discussions-To: Distutils SIG
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

distutils-sig <distutils-sig at python.org>

pep-9999.txt Outdated
Last-Modified: $Date$
Author: Dustin Ingram <di@di.codes>
Discussions-To: Distutils SIG
Status: Accepted
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initial state should be Draft

pep-9999.txt Outdated
Content-Type: text/x-rst
Created: 1-Dec-2017
Python-Version: 3.x
Post-History:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaces: 345

pep-9999.txt Outdated

* Metadata-Version is now 1.3.

* Fields are now specified via the `Core Metadata Specification`_.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Additional changes to cover:

Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain why you think this PEP should explicitly include the two new fields? I opted not to because I felt that it somewhat defeats the purpose of this PEP (i.e. that new fields are now specified elsewhere, not in this PEP.)

Do you think this PEP should include the entire specification for the two new fields, or just mention here (in the "Summary of Differences" section) that they have been added?

In your mind, if we needed to add another field after the acceptance of this PEP, what would happen? Would we amend this PEP? Or would that field be part of "Metadata 1.4"? Would a new metadata version require a new PEP?

Along those lines, should this PEP mention something about when and why the metadata version number should change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal of the process change is to switch to a model where distutils-sig PEPs describe changes to the specifications, but are not themselves used as primary reference documents. The big difference that makes is that it means we don't copy and paste unchanged fields into the new PEP - we only list the new & updated ones.

Since this is a backfill PEP, you're creating a record of past decisions already made in GitHub PRs rather than actually requesting approval of the changes, but in any future metadata update PEPs will also need to make the argument for why particular additions or changes are needed.

For moving the reference URLs to PyPUG, the appropriate reference would be: pypa/pypa.io#11

For Description-Content-Type the appropriate reference would be: pypa/packaging.python.org#258

For Provides-Extra, the most appropriate reference would be probably be to an early draft of PEP 426, such as: https://github.com/python/peps/blob/493ff3b7b5e6f83e7ebaf6ac8902fcdc8835abbc/pep-0426.txt#L428

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A shorter version of that comment: rather than being about what the field is, the entry in the PEP should focus on why we added it.

Provides-Extra: so installers know which extras are available for selection, and so publishing tools can check for typos in environment markers.
Description-Content-Type: so tools (most notably PyPI) can be told how to render the long description, rather than always assuming the use of reStructuredText

@ncoghlan
Copy link
Collaborator Author

ncoghlan commented Dec 2, 2017

I know you said you'd like to keep this as simple as possible, but I think it will be worth our while to define a canonical transformation from the current line-oriented email-header style formatting (which requires reading the PEP in with email.parser.HeaderParser and accounting for multiple use fields yourself) and a JSON-compatible data structure.

As a first draft of that:

  1. The existing key-value format should be read with email.parser.HeaderParser
  2. All keys should be reduced to lower case, but otherwise left alone
  3. Multiple-use fields should be collected into a single list value for that key
  4. Keywords should be converted to a list by splitting on whitespace
  5. The result should be stored as a string-keyed dictionary

@di
Copy link
Owner

di commented Dec 4, 2017

Thanks very much for the comments, @ncoghlan. I just have a handful of questions and then I should have another pass at this for you.

ericvsmith and others added 8 commits December 4, 2017 05:37
Mention an updated schedule for non-provisional status and the end of
non-typing annotations in PEP 563, and link there for the __future__
statement as well.
Fix also a typo in the abstract: remove "+".
@di
Copy link
Owner

di commented Dec 4, 2017

@ncoghlan This is ready for another review.

Naddiseo and others added 2 commits December 4, 2017 13:49
Fix 1 bug. Enhance handling of trailing whitespace for URLs. Correct spelling.

Add tests.
Copy link
Collaborator Author

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

This version looks pretty good to me as a starting point for distutils-sig discussion.

If you turn it into a PEP 566 PR against the main PEPs repo, then I can merge it, and you can start the thread on distutils-sig (that should hopefully be short, but folks may spot holes in the proposed JSON conversion algorithm)

pep-9999.txt Outdated

This PEP describes a mechanism for adding metadata to Python distributions.
It includes specifics of the field names, and their semantics and
usage.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This opening paragraph should change. Something like:

This PEP describes the changes between versions 1.2 and 1.3 of the core metadata specification for Python packages. It also changes to the canonical source for field specifications to the Core Metadata Specification_ reference page on packaging.python.org.

And then adjust the rest of the abstract accordingly (e.g. while mentioning PEP 345 is good, I don't think you need to mention the others)

pep-9999.txt Outdated
JSON-compatible Metadata
========================

It may be necessary to represent metadata into a data structure which does not
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Grammar here isn't quite right. Perhaps "... to store metadata in a ..."?

@di di closed this Dec 5, 2017
@di di deleted the metadata-1.3 branch December 5, 2017 16:15
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.

8 participants