| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks @ngoldbaum for writing docs for the Limited API! Overall this content looks like a great start. My main comment right now is that it's not in the right places: it can't be in the reference guide, and the examples section of the "Passing arguments to Meson" page also feels wrong. I think we need a new page, as a how-to under "Using shared libraries" called something like "Using the Limited C API" or some such title.
This content, put together in a single page, will be a great start. I have a few things I'd like to add, like how to set it up as opt-in (e.g., as in PyWavelets/pywt#828).
We can then also address a couple of open issues: gh-624, some parts of gh-787.
Sorry, something went wrong.
|
I rearranged with a new howto guide page. Let me know what you think. |
Sorry, something went wrong.
There was a problem hiding this comment.
Just a few quick remarks. I find the content in the new howto page hard to read, mostly because there are things repeated several times with different language and because the content is not presented at increasing level of detail but zooming in and out constantly. The other are minor comments. Please squash the commits into just one commit.
Sorry, something went wrong.
|
@dnicolodi thanks! I've applied your suggestions. I also rearranged the howto page and I think it's clearer now. |
Sorry, something went wrong.
There was a problem hiding this comment.
This is looking pretty good. A couple more suggestions to give the desired recommendations to users.
Sorry, something went wrong.
|
Thanks for the comments Ralf! I think I addressed them. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks. This looks very nice. A few more comments.
Sorry, something went wrong.
|
Thanks for all of the detailed suggestions! I think I've applied them. I also had a private chat with Ralf where he explained to me that the wheel interpreter compatibility tag being different from the target API is an intentional design choice in meson-python and not just an accident of how support for limited API builds is implemented. I now phrase that as meson-python "conservatively chooses" this to make that clear. I also split out all the compatibility details into their own subsection after the main build configuration discussion. Now whenever people have questions about that point in the future, you can point to a specific docs page about it! I also marked the issue as explicitly fixing #624 and updated the PR description to reflect the final state. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks Nathan. Very close, two more comments from me.
Sorry, something went wrong.
There was a problem hiding this comment.
This looks great to me now. I checked and resolved all open comments. I've already been pointing people at these docs, so I'll merge this in a few days unless there are more comments.
Sorry, something went wrong.
All suggestions from this review were accepted/implemented.
|
Thanks again @ngoldbaum, and thanks @dnicolodi for the reviews. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Adds a new howto guide on targeting the CPython limited API and producing stable ABI wheels.
Fixes #624