-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add template dynamic-metadata plugin and support requesting other fields #1047
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
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
6f2b676 to
2b45a9d
Compare
Signed-off-by: Henry Schreiner <[email protected]>
2b45a9d to
c2c2dd2
Compare
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.
Pull Request Overview
This PR implements a new template dynamic‐metadata plugin and adds support for dynamic_metadata_needs, addressing issue #21. Key changes include:
- Updating tests and plugin configuration to support optional dependencies templating.
- Introducing new functions in the template plugin and adjusting existing plugins (regex and fancy_pypi_readme) for compatibility.
- Refactoring provider loading and dependency resolution via TopologicalSorter.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_dynamic_metadata.py | Added tests for optional dependencies and updated expected readme output. |
| tests/packages/dynamic_metadata/plugin_project.toml | Extended dynamic metadata configuration to include optional-dependencies with template support. |
| src/scikit_build_core/metadata/template.py | Added template dynamic metadata processing and its corresponding dynamic_metadata_needs function. |
| src/scikit_build_core/metadata/regex.py | Refactored to delegate formatting to _process_dynamic_metadata and reduce code duplication. |
| src/scikit_build_core/metadata/fancy_pypi_readme.py | Updated dynamic_metadata signature and added a new function (dynamic_requires_needs). |
| src/scikit_build_core/metadata/init.py | Extended _process_dynamic_metadata to handle additional metadata fields. |
| src/scikit_build_core/builder/_load_provider.py | Refactored dynamic metadata loading and dependency extraction using TopologicalSorter. |
| src/scikit_build_core/build/metadata.py | Added backward compatibility for dynamic_metadata providers and updated provider import. |
| pyproject.toml | Added dependency for graphlib_backport and updated mypy overrides. |
| docs/configuration/dynamic.md | Documented new templating functionality in dynamic metadata configuration. |
Files not reviewed (1)
- docs/api/scikit_build_core.metadata.rst: Language not supported
Comments suppressed due to low confidence (2)
src/scikit_build_core/build/metadata.py:53
- [nitpick] Consider adding a comment explaining the fallback mechanism using inspect.signature for backward compatibility with providers that do not accept a metadata dict.
sig = inspect.signature(provider.dynamic_metadata)
src/scikit_build_core/builder/_load_provider.py:87
- [nitpick] Consider adding an inline comment to clarify the purpose of converting the output of dynamic_metadata_needs into a frozenset for dependency ordering.
needs = frozenset(loaded_provider.dynamic_metadata_needs(field, config) if isinstance(loaded_provider, DynamicMetadataNeeds) else [])
|
I'd like to consider an alternative, that probably can't be supported here, but could be supported by [[tool.dynamic-metadata]]
field = "version"
provider = "..."
[[tool.dynamic-metadata]]
field = "readme"
provider = "..."This would be guaranteed to always run in order top to bottom, so topological sorting is not needed. This would not have a nice path to standardization, though. A small downside is that a user then has to get the order correct, but it could be an error if they don't, and you could chain these (ideal for partially dynamic metadata). |
Signed-off-by: Henry Schreiner <[email protected]>
2adfa37 to
cbddac0
Compare
316f1bb to
f30cd58
Compare
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
ef33252 to
69e8424
Compare
Signed-off-by: Henry Schreiner <[email protected]>
Also adding back a couple of test files that I wrote but missed adding in previous PRs: * #1051 * #1047 Signed-off-by: Henry Schreiner <[email protected]>
|
@henryiii Can I already use this in my project as the new functionality in dynamic-metadata is not releases yet? |
|
Technically if you require version 0.11.2 or higher, yes. The dynamic-metadata dependency is not being used, the implementation is replicated inside this project. At least it is always good to get the feedback from early adopters. More edit: There is one point to consider is that interface of |
|
Excerpt from my pyproject.toml: [build-system]
requires = ["scikit-build-core", "dynamic-metadata"]
build-backend = "scikit_build_core.build"
[project]
...
dynamic = ["version", "scripts"]
[tool.scikit-build]
...
experimental = true
minimum-version = "0.11.2"
metadata.version.provider = "scikit_build_core.metadata.setuptools_scm"But this still uses 0.1.0 of dynamic-metadata: |
|
|
|
@LecrisUT Never mind, I was trying this because I got the error (without using dynamic-metadata): But it seems like the |
|
Hmm, indeed it seems so, but not sure if it should be |
|
This is stable/usable, it's just adding new plugins that's experimental (you'll have to set the |
Fixes issue described in #1047 (comment). --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Cristian Le <[email protected]> Co-authored-by: Henry Schreiner <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Implementation of scikit-build/dynamic-metadata#21. Mentioned in https://discuss.python.org/t/partially-dynamic-project-metadata-proposal-pre-pep/88608.
TODO: support specifying a non-dynamic field. It should not be an error if it exists already!