-
-
Notifications
You must be signed in to change notification settings - Fork 827
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
PEP 689: Add docs for unstable C API #1060
Conversation
.. _unstable-capi: | ||
|
||
Unstable C API | ||
============== |
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'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?
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.
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.
And if you're not reading from top to bottom, does the order matter?
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, 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.:
- Limited API
- Guidelines for changing the Limited API
- ...
- Public C API
- C API Tests
- Unstable C API
- Moving an API from the public tier to Unstable
- ...
- Internal API
- With PyAPI_FUNC or PyAPI_DATA
- ...
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.
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.
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 brought it up here since it was this PR that introduced the regression on the ordering (it previously was ordered least to most portable, which IMO isn't perfect but much better than all over the place), and my initial suggestion was to just insert the category at the appropriate point in that order, i.e. above Public API.
However, as @encukou brought up the added description of the unstable API substantially refers to that of the Public API, and as you mentioned this change would also be best made as a separate PR given, as I'd also noticed, it would conflate the two distinct changes (with squash-merge in use). Ideally, that change would have been considered and implemented first to avoid introducing a regression here and only fixing it later, but pragmatically speaking at this point I guess we just go ahead with this just fixing the tier list order (per the comment below) while deferring the section order to a followup PR.
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.
That sounds like a good order for the user-facing docs. Developers who implement the API generally go in the opposite direction.
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 |
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.
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)
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.
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").
developer-workflow/c-api.rst
Outdated
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.
Choose a reason for 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 |
Co-authored-by: C.A.M. Gerlach <[email protected]> Co-authored-by: Ezio Melotti <[email protected]>
@@ -4,18 +4,20 @@ | |||
Changing Python's C API | |||
======================= | |||
|
|||
The C API is divided into three sections: | |||
The C API is divided into these tiers: |
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.
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).
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 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.
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.
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).
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.
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”.
developer-workflow/c-api.rst
Outdated
The old name should continue to be available until an incompatible change is | ||
made. |
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 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. |
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 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.
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.
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.
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.
@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.
DeprecationWarning
is the usual warning category to use, butPendingDeprecationWarning
may 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.
[...]
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.
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.
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.
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.
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'll need to run it by the SC.
(I plan to do this after merging, and revert if necessary)
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 :( |
I'm back! Does this look good? |
Thank you for the reviews! |
No description provided.