PEP 517: remove prepare_wheel_metadata references #311
Conversation
- replace with prepare_metadata_for_build_wheel where appropriate - update the API evolution example to be based on build_sdist rather than prepare_wheel_metadata - also clarified the frontend example code covering one way to handle prepare_metadata_for_build_wheel being optional
ncoghlan
force-pushed
the
pep-517-update-example-backend
branch
from
eb88c49
to
8246224
Compare
| back onto ``prepare_wheel_metadata`` otherwise; and we want it to be | ||
| ``build_sdist_from_vcs`` hook, which provides an alternative to the current | ||
| ``build_sdist`` hook where the frontend is responsible for passing | ||
| version control tracking metadata to backends (including indicating when all |
There was a problem hiding this comment.
This might be more realistic than the example it replaces, but I think that trying to understand it is more likely to distract the reader from the point we're making here about why a Python API is nicer than a command line API. The more abstract 'different metadata format' seems clearer to me.
There was a problem hiding this comment.
I see your point, but I don't even want to put the suggestion out there that we might be thinking about changing the wheel metadata format.
Honestly, given the rejection of PEP 516, it might make sense to cull this whole section severely, as it's mainly a distraction know.
There was a problem hiding this comment.
+1 to cutting this comparison section down to a brief paragraph, or moving it to an appendix or something (can PEPs have appendices?). It's a large amount of text which doesn't really help to describe what the PEP is.
There was a problem hiding this comment.
Yes, PEPs can definitely have appendices - they still appear in the same document, they're just moved to the end where they're more clearly out of the way.
There was a problem hiding this comment.
I'll note that's a bigger structural change than I'm prepared to draft as BDFL-Delegate, though - it's subjective enough that I think it's best handled directly by the PEP authors, rather than treating it as a reviewer proposed fix or clarification.
There was a problem hiding this comment.
OK, I'll have a go at that once your new PR is merged.
pep-0517.txt
Outdated
| copy(metadata_dir.parent / *.whl, output_dir) | ||
| else: | ||
| backend.build_wheel(output_dir, config_settings, metadata_dir) | ||
| already_built = metadata_dir.parent / "ALREADY_BUILT_WHEEL" |
There was a problem hiding this comment.
This implies that we're passing pathlib objects to hooks. I personally like working with pathlib, but I think that specifying that would get messy for Python versions which don't have the pathlike protocol, and even worse for those which don't have pathlib in the standard library.
So I'm inclined to use plain string paths throughout the spec and all examples.
There was a problem hiding this comment.
That's a different problem with this example - these aren't hook implementations, they're internal helper functions in a frontend.
I was momentarily confused when updating the example, and now that you've been confused in reviewing, I think that's a sign I should definitely change the function names.
There was a problem hiding this comment.
I checked in an update to change the function names, so I'll go ahead and merge the PR so the PEP is self-consistent again.
Re-using the hook names in an example of calling the hooks is confusing, so don't do that. Also update these example helper functions to accept the current build backend as an explicit parameter.
| wheel_fname = already_built.read_text() | ||
| copy(metadata_dir.parent / wheel_fname, output_dir) | ||
| else: | ||
| backend.build_wheel(output_dir, config_settings, metadata_dir) |
There was a problem hiding this comment.
The functions you're defining in this example are not hook implementations, but they still pass Path objects to the hooks, which is not compliant with the current spec as I understand it. You could str() them when you call the hooks, but I think it would be clearer to consistently use string paths in examples.
There was a problem hiding this comment.
Good point - I'll switch it over to using os.path functions and builtin open().
There was a problem hiding this comment.
New PR for this change: #312
rather than prepare_wheel_metadata
handle prepare_metadata_for_build_wheel being optional