-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Infra: Use a list of authors in peps.json
#4226
base: main
Are you sure you want to change the base?
Conversation
I think the first question is if we want to break backwards compatibility? I lean to not breaking compatibility. If so, the other options are:
|
I'd vote for keeping |
From a cursory search, it seems the vast majority of instances of But sure, happy to add as a new field. The only negative would be the file size (~15% increase). A |
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.
From a cursory search, it seems the vast majority of instances of
peps.json
in the wild are reproductions of the 723 example:
Here's a couple that would have broken (but I'm sure we could have convinced the authors to update :)
- https://github.com/brettcannon/brettcannon/blob/42eb826a969bd9430053193e3fb5f74a43b74f77/free-labour.py#L309
- https://github.com/JacobCoffee/byte/blob/336008fe8c42850d313c912f8e34a92bb02bffe4/byte_bot/byte/lib/utils.py#L430
But sure, happy to add as a new field. The only negative would be the file size (~15% increase).
Yep, I think 331K -> 368K is okay. Thanks!
"""Returns all headers of the PEP as a dict.""" | ||
return { | ||
"number": self.number, | ||
"title": self.title, | ||
"authors": ", ".join(author.full_name for author in self.authors), | ||
"authors": ", ".join(self._author_names), |
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.
Let's group them together:
"authors": ", ".join(self._author_names), | |
"authors": ", ".join(self._author_names), | |
# extra non-header keys for use in ``peps.json`` | |
"author_names": tuple(self._author_names), |
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.
This was intentional, as the keys are all of the headers from the PEP in order -- the "extra" keys (url etc) are sorted at the end.
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.
Hmm, these are essentially the same header, just in a different format?
There is a read-only JSON document of every published PEP available at | ||
https://peps.python.org/api/peps.json or https://peps.python.org/peps.json | ||
(both URLs point to the same document). |
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.
Ah, this is a bug, it should only be at https://peps.python.org/api/peps.json and not https://peps.python.org/peps.json
Re: #2584 (comment)
There is a read-only JSON document of every published PEP available at | |
https://peps.python.org/api/peps.json or https://peps.python.org/peps.json | |
(both URLs point to the same document). | |
There is a read-only JSON document of every published PEP available at: | |
* https://peps.python.org/api/peps.json |
Let's remove the one at the root: #4231.
A GitHub code search only shows results for the correct https://peps.python.org/api/peps.json.
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 suggest moving this bit to its own PR (which I'll oppose ;) and keep this PR focused on the new author_names
(which I'll approve)?
peps.json
peps.json
Closes #4211.
Alternatively, we could just use a different seperator (
;
)? Theauthors
field ofpeps.json
was originally intended for literal reproduction without any re-parsing, hence why I joined the alreay-parsed list of authors into a string.A
📚 Documentation preview 📚: https://pep-previews--4226.org.readthedocs.build/peps.json