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

Replace some PEP references with internal references #1405

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

The versions of pip and setuptools in question date back to 2014.
@jeanas
Copy link
Contributor Author

jeanas commented Dec 7, 2023

I have fixed a bunch of merge conflicts with main and added a few more replacements.

Comment on lines 135 to 137
<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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

True for the old content, but I think that when adding new content, we could use the better role right away, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (used :file:).

source/glossary.rst Outdated Show resolved Hide resolved
source/guides/packaging-namespace-packages.rst Outdated Show resolved Hide resolved
source/tutorials/installing-packages.rst Show resolved Hide resolved
@jeanas
Copy link
Contributor Author

jeanas commented Dec 7, 2023

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.

@jeanas
Copy link
Contributor Author

jeanas commented Dec 20, 2023

@webknjaz Anything missing here?

@webknjaz
Copy link
Member

webknjaz commented Dec 22, 2023

(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.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@webknjaz webknjaz Dec 22, 2023

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.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

Comment on lines 371 to 374
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.
Copy link
Member

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.

Comment on lines 111 to 112
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.
Copy link
Member

Choose a reason for hiding this comment

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

Note for myself: reflow.

Comment on lines 109 to 110
extras is formally specified in the :ref:`dependency specifier specification
<dependency-specifiers>` (as ``extras``) and restrictions on values specified
Copy link
Member

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
Copy link
Member

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.

Comment on lines 77 to 79
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>`):
Copy link
Member

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
Copy link
Member

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.

Comment on lines 138 to 139
projects currently is something that contains a :term:`pyproject.toml`
or :term:`setup.py` file at the root of the project source directory.
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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? 🤔

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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

Comment on lines 124 to 127
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.
Copy link
Member

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

@webknjaz
Copy link
Member

webknjaz commented Dec 22, 2023

@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 setup.py w/o pyproject.toml in that sentence. Let's try correcting that before getting this PR merged!

@jeanas
Copy link
Contributor Author

jeanas commented Dec 22, 2023

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).

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...

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...

Ok, I will try to be more careful with this. (I have a compulsive habit of pressing M-q in Emacs.)

@jeanas
Copy link
Contributor Author

jeanas commented Dec 22, 2023

@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.
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.

2 participants