GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — PEP 689: Add docs for unstable C API by encukou · Pull Request #1060 · python/devguide Conversation
CAM-Gerlach
self-requested a review
5 of 13 tasks
CAM-Gerlach
reviewed
Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+148
to
+151
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm rather confused why this was added between Public C API and Limited C API, instead of between internal Internal API and Public C API. Isn't it specifically stated to be a step between the latter two, not the former two, so shouldn't it be moved there instead?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning is that the description builds on top of the public API section. If you're reading the whole document, the references would make you jump around so you'd end up with the order in this PR.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I see. I understand the rationale there, though IMO heading order still matters both for folks consulting specific sections/subsections (which are likely the majority of readers) and those reading the whole document sequentially, as it establishes a clear order and hierarchy of the API levels and the desired precedence for usage (limited, public, unstable, internal) rather confusingly implying a structure that doesn't reflect their actual conceptual organization, and requiring users to read the full text to ensure they know the correct order. In light of that, since the Limited API section doesn't really directly refer much to the Public C API section and is the highest-level tier (rather than the lowest, as the current order implies), what about instead switching it with the Internal API, which would make the order of the page from highest-level tier to lowest-level tier, keeping the Unstable API listed after Public C API as you have it now, but making a lot more sense overall. I.e.:
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The suggested restructuring sounds plausible to me, but also feels like it would be better served by being done as its own PR rather than both adding new content and restructuring the existing content in this PR.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
|
Freaking GitHub, every long review I've done lately it's silently auto-submitted it (without the top level review comment) right the middle of the review for no obvious reason, with no special keyboard shortcut on my end (as far as I'm aware) and no changes on the PR's end. My comments still show up as pending in the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Add remaining comments as review suggestions.
(Standard reminder: You can directly apply all the suggestions you want in one go with Files changed -> Add to batch -> Commit)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
An additional dose of nitpickery on top of CAM's comprehensive review.
Some of the comments can be applied throughout the page, even if I didn't report all the occurrences (e.g. quotes, capitalization, "What's New").
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| Note that historically, underscores were used for API that is better served by | ||
| the :ref:`unstable-capi`: | ||
|
|
||
| * “provisional” API, included in a Python release to test real-world |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| * “provisional” API, included in a Python release to test real-world | |
| * "provisional" API, included in a Python release to test real-world |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: C.A.M. Gerlach <[email protected]> Co-authored-by: Ezio Melotti <[email protected]>
| ======================= | ||
|
|
||
| The C API is divided into three sections: | ||
| The C API is divided into these tiers: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Same comment with the order here, only more crucial—"tiers" explicitly denotes an ordered hierarchy, and yet internal API is listed and numbered first, then public API, then unstable API, then Limited API, which directly implies that this is the order of the tiers. Swapping "internal" and "limited" would fix this (as would swapping "general" and "unstable", as I originally suggested, but wouldn't match the order in the document, and going from highest-level/most recommended to lowest-level/least recommended makes more sense anyway).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think the ordering issue is real, but I think trying to resolve it directly in this PR will make it harder to review the content changes.
My suggestion would be to instead file a docs enhancement request to restructure the API tier docs to run from either most-to-least or least-to-most portable, rewording the individual sections as necessary.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yeah, while is this PR that is introducing a regression (as the order currently is least-to-most portable), practically speaking the approach you suggest is the most straightforward to implement at this point.
That being said, we should at least try to avoid introducing too much of an unnecessary regression here, particularly in this list where the regression is more serious but much easier to fix. Therefore, to help mitigate this while minimizing the further changes here, the simplest fix is to just add the unstable tier to its correct place in this explicitly ordered tier list per the current least-to-most portable order (while keeping the document section order as it is now in this PR).
That greatly reduces the potential confusion while not requiring any additional changes, and makes much more sense than trying to match the section order since this is an ordered list of API tiers, not sections on the page (that's what the ToC is for, and the page section order not being as important is the justification for the regression in the first place).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable, thanks.
FWIW, the original order matches an ideal of how APIs are “promoted” as they mature: first implement the functionality, then make it usable for others, then make sure it's rock-solid.
In this list, the unstable API is on the same level as the general public API. And it's a niche enough topic to be documented with “same as that except for the following”.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| The old name should continue to be available until an incompatible change is | ||
| made. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I have no subject matter expert opinion on the changes, of course (though ISTM that removing the alias after a backward-incompatible change to a public API should still be at least mentioned, as it is safer than silently changing behavior) but if you want to accept it, I've made @ncoghlan 's comment from the above into an actual suggestion, on the correct lines, and with some copyediting cleanup to make it more concise and avoid repetition.
| The old name should continue to be available until an incompatible change is | |
| made. | |
| The previous unstable name should remain available as long as the new public name is. | |
| If the public name is ever subsequently deprecated, its unstable alias should be too. | |
| While the unstable name could theoretically be removed or deprecated immediately without | |
| violating the expectations set for the unstable API tier, there's also no need to break working | |
| code just because a previously unstable API is now considered ready for a wider audience. |
Or, with an explicit mention of removal after an incompatible change, as before:
| The old name should continue to be available until an incompatible change is | |
| made. | |
| The previous unstable name should remain available as long as the new public name is, | |
| unless an incompatible change is made to the public API that could break users anyway. | |
| If the public name is ever subsequently deprecated, its unstable alias should be too. | |
| While the unstable name could theoretically be removed or deprecated immediately without | |
| violating the expectations set for the unstable API tier, there's also no need to break working | |
| code just because a previously unstable API is now considered ready for a wider audience, | |
| unless it may break or change its behavior anyway due to another incompatible change. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It's a public API, so incompatible changes are not allowed. This new text should not suggest that they are. If we want to make an incompatible change to a public API, we have to add a new API and deprecate the old one instead.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, thanks—as mentioned, my C-API expertise is non-existent. For my future reference, where's that documented again? I don't see where that is explicitly mentioned in the C API stability policy, PEP 387 or this document, which are the places I would expect it to be.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@CAM-Gerlach It's basically the entirety of the "Making incompatible changes" section of PEP 387. (Particularly steps 2 and 3.)
Making incompatible changes
Making an incompatible change is a gradual process performed over several releases:
- Discuss the change. Depending on the degree of incompatibility, this could be on the bug tracker, python-dev, python-list, or the appropriate SIG. A PEP or similar document may be written. Hopefully users of the affected API will pipe up to comment.
- Add a warning. If behavior is changing, the API may gain a new function or method to perform the new behavior; old usage should raise the warning. If an API is being removed, simply warn whenever it is entered.
DeprecationWarningis the usual warning category to use, butPendingDeprecationWarningmay be used in special cases where the old and new versions of the API will coexist for many releases [2]. Compiler warnings are also acceptable. The warning message should include the release the incompatibility is expected to become the default and a link to an issue that users can post feedback to.- Wait for the warning to appear in at least two minor Python versions of the same major version, or one minor version in an older major version (e.g. for a warning in Python 3.10, you either wait until at least Python 3.12 or Python 4.0 to make the change). It’s fine to wait more than two releases.
[...]
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Well, its certainly not "the entirety of the section"; it's the very specific point above I'm after. On careful inspection I do see what specifically implies this, though it is a bit buried and took multiple reads to find:
If behavior is changing, the API may gain a new function or method to perform the new behavior; old usage should raise the warning.
The language here really should be more clear and explicit, as the "may"s and "should"s used here certainly don't send nearly the same message as "incompatible changes are not allowed" and "If we want to make an incompatible change to a public API, we have to add a new API and deprecate the old one instead." Also, "old usage" is somehwt unclear terminology. For that, it should instead state something like
If an incompatible behavior change to an existing API is needed, a new function or method MUST be added and the old construct deprecated, so existing uses don't silently break.
To note, that isn't always true at least for Python APIs; I can recall several instances (e.g. regex group identifiers) where incompatible changes were made to existing functions for various reasons.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think the alias should be removed as soon as the public API is deprecated. If you signed up for updating your code to react to implementation changes, you should update it when the API is no longer favoured.
| The old name should continue to be available until an incompatible change is | |
| made. | |
| The old name should remain available until the | |
| new public name is deprecated or removed. |
This is technically a change in the PEP, I'll need to run it by the SC.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'll need to run it by the SC.
(I plan to do this after merging, and revert if necessary)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I will get back to this, but I'm catching up after COVID and the deadline for 3.12 features is coming closer, so it might be another few weeks :( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm back! Does this look good? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thank you for the reviews! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.