-
Notifications
You must be signed in to change notification settings - Fork 933
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
Replace some PEP references with internal references #1405
base: main
Are you sure you want to change the base?
Conversation
The versions of pip and setuptools in question date back to 2014.
1dad7b1
to
b746b22
Compare
I have fixed a bunch of merge conflicts with |
b746b22
to
79385f3
Compare
source/glossary.rst
Outdated
<pyproject-guide-build-system-table>` of their ``pyproject.toml`` file | ||
(or the deprecated practice of having no ``pyproject.toml`` but either | ||
``setup.cfg`` or ``setup.py``), another practical way to define projects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While in it, please use the :file:
role for inline highlighting.
Also, the new text implies that having just setup.cfg
works. But it doesn't. It only works in pair with either PEP 517 or the deprecated setup.py
CLI invocation. Otherwise, there's simply nothing that would attempt reading it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are less ocurrences of :file:`pyproject.toml`
than ``pyproject.toml``
. I'd rather keep this PR focused on PEP references, we can make it more consistent globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good remark about setup.cfg
, it looks like neither pip nor build accepts a source tree without either pyproject.toml
or setup.py
. I'll change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are less ocurrences of
:file:`pyproject.toml`
thanpyproject.toml
. I'd rather keep this PR focused on PEP references, we can make it more consistent globally.
True for the old content, but I think that when adding new content, we could use the better role right away, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (used :file:
).
The CI failure is a spurious linkcheck failure. Edit: in fact the workflow triggered by the PR succeeded, although the one triggered by the push failed. |
@webknjaz Anything missing here? |
(general contribution process feedback) @jeanas sorry, I make review rounds once in a while and sometimes miss things. Plus, it's hard to find the silently resolved threads, which results in more effort required to find and go through them (this is the primary reason for having thread authors resolve them or causing resolution notifications at least). |
- Corresponding :ref:`core metadata <core-metadata>` field: | ||
:ref:`Requires-Dist <core-metadata-requires-dist>` and | ||
:ref:`Provides-Extra <core-metadata-provides-extra>` | ||
|
||
The (optional) dependencies of the project. | ||
|
||
For ``dependencies``, it is a key whose value is an array of strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(general contribution process feedback)
Try not to reflow lines that don't change the content: it's hard to compare what changed, if an entire block of 13 lines is marked as updated — this causes an unnecessary cognitive load for the reviewers trying to compare strings byte-by-byte...
This is the reason it's recommended to split the formatting changes from the functional ones in normal code projects, but I'm sure it's also applicable to the docs.
Here's a collection of materials related to review processes, I enjoyed reading — maybe you'll find some insights for yourself too:
:ref:`Requires-Dist <core-metadata-requires-dist>` entry for the | ||
matching :ref:`Provides-Extra <core-metadata-provides-extra>` | ||
metadata. | ||
For ``dependencies``, it is a key whose value is an array of strings. Each |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: this line doesn't have content changes, it just has an extra trailing word that was previously on the following line.
matching :ref:`Provides-Extra <core-metadata-provides-extra>` | ||
metadata. | ||
For ``dependencies``, it is a key whose value is an array of strings. Each | ||
string represents a dependency of the project and MUST be formatted as a valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: no changes here either.
metadata. | ||
For ``dependencies``, it is a key whose value is an array of strings. Each | ||
string represents a dependency of the project and MUST be formatted as a valid | ||
:ref:`dependency specifier <dependency-specifiers>`. Each string maps directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: the actual update from :pep:
to :ref:
. Plus an inter-sentence whitespace doubled.
For ``dependencies``, it is a key whose value is an array of strings. Each | ||
string represents a dependency of the project and MUST be formatted as a valid | ||
:ref:`dependency specifier <dependency-specifiers>`. Each string maps directly | ||
to a :ref:`Requires-Dist <core-metadata-requires-dist>` entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: no content changes.
:ref:`dependency specifier <dependency-specifiers>`. Each string maps directly | ||
to a :ref:`Requires-Dist <core-metadata-requires-dist>` entry. | ||
|
||
For ``optional-dependencies``, it is a table where each key specifies an extra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: no content changes here, it just moved two extra trailing words from the following line.
to a :ref:`Requires-Dist <core-metadata-requires-dist>` entry. | ||
|
||
For ``optional-dependencies``, it is a table where each key specifies an extra | ||
and whose value is an array of strings. The strings of the arrays must be valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: no content changes here, just reflow.
|
||
For ``optional-dependencies``, it is a table where each key specifies an extra | ||
and whose value is an array of strings. The strings of the arrays must be valid | ||
:ref:`dependency specifiers <dependency-specifiers>`. The keys MUST be valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: ref update; double whitespace.
values for :ref:`Provides-Extra <core-metadata-provides-extra>`. Each value in | ||
the array thus becomes a corresponding :ref:`Requires-Dist | ||
<core-metadata-requires-dist>` entry for the matching :ref:`Provides-Extra | ||
<core-metadata-provides-extra>` metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: reflow; double whitespaces.
in :pep:`685`. For tools writing the file, it is recommended only to insert a space | ||
between the object reference and the left square bracket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: reflow.
extras is formally specified in the :ref:`dependency specifier specification | ||
<dependency-specifiers>` (as ``extras``) and restrictions on values specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: ref updates.
@@ -102,7 +102,7 @@ not only provides features that plain :ref:`distutils` doesn't offer | |||
also provides a consistent build interface and feature set across all | |||
supported Python versions. | |||
|
|||
Consequently, :ref:`distutils` was deprecated in Python 3.10 by :pep:`632` and | |||
Consequently, :ref:`distutils` was deprecated in Python 3.10 (by :pep:`632`) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself: style change — parens.
namespace package is that you just omit :file:`__init__.py` from the | ||
namespace package directory. An example file structure (following | ||
:ref:`src-layout <setuptools:src-layout>`): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: reflow only.
:ref:`setuptools`, another practical way to define projects currently | ||
is something that contains a :term:`pyproject.toml`, :term:`setup.py`, | ||
or :term:`setup.cfg` file at the root of the project source directory. | ||
Since projects create :term:`Distributions <Distribution Package>` using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: removed "most" + reflow.
source/glossary.rst
Outdated
projects currently is something that contains a :term:`pyproject.toml` | ||
or :term:`setup.py` file at the root of the project source directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we improve this so that it doesn't imply a suggestion to use setup.py
w/o pyproject.toml
? Also, I'd really like a mention of setup.cfg
to be kept in the context of declarative metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two lines before, it says “(or with the deprecated practice of having a setup.py
without an accompanying pyproject.toml
)”, is this is not enough? Maybe I should put “deprecated” in italics?
even include multiple platform-specific Python distributions, meaning that a | ||
single pex file can be portable across Linux and macOS. pex is released under the | ||
Apache License 2.0. | ||
`pex <https://pypi.org/project/pex/>`__ is a library for generating .pex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: I don't understand what changed on this line. Using Ctrl+F
highlights both old an new versions the same way. Can it be some non-printable whitespace? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a double space removed in is a library
. Let me split that in its own commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I ended up leaving it, it's not really important. It had been removed by my editor.)
single pex file can be portable across Linux and macOS. pex is released under the | ||
Apache License 2.0. | ||
`pex <https://pypi.org/project/pex/>`__ is a library for generating .pex | ||
(Python EXecutable) files which are executable Python environments in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: reflow only
Apache License 2.0. | ||
`pex <https://pypi.org/project/pex/>`__ is a library for generating .pex | ||
(Python EXecutable) files which are executable Python environments in | ||
the spirit of virtualenvs. pex is an expansion upon the ideas found in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: wording change
applications as simple as cp. pex files may even include multiple | ||
platform-specific Python distributions, meaning that a single pex file | ||
can be portable across Linux and macOS. pex is released under the Apache | ||
License 2.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: reflow only
@jeanas another round of review complete. I marked most of the files as "viewed" on the GH UI so if you don't change them, they won't show up for me in the next rounds. I made some self-notes to help myself understand if there were any changes in some places, but those are non-actionable. I think the only concern is regarding the same paragraph: https://github.com/pypa/packaging.python.org/pull/1405/files#r1435296663 — reading the wording made me feel like it implies the use of |
Sorry — I never quite know what convention to use for this. On most projects that I personally work on, it is usual for PR authors to resolve threads that they (believe to) have addressed, so that it is easy to see what the outstanding issues are without writing "Done" on each thread. I think this varies widely between projects...
Ok, I will try to be more careful with this. (I have a compulsive habit of pressing M-q in Emacs.) |
9d745ba
to
b96c506
Compare
@webknjaz I have greatly minimized the diff, let me know if it's better. |
Use internal references when the PEPs have been imported into the PUG.
b96c506
to
b1c476c
Compare
Use internal references when the PEPs have been imported into the PUG.
📚 Documentation preview 📚: https://python-packaging-user-guide--1405.org.readthedocs.build/en/1405/