feat(api): pass local version label to build backend interface #814
Conversation
Reviewer's Guide by SourceryThis pull request introduces the ability to pass a local version label to the build backend via 'config_settings', as per PEP 517. The implementation includes modifications to the Builder class to handle configuration settings and apply local version labels during package builds. Additionally, comprehensive tests have been added to ensure the correct functionality of building wheels, sdists, and editable wheels with local version labels. Sequence diagram for local version label application during buildsequenceDiagram
participant Client
participant Builder
participant Poetry
participant Package
Client->>Builder: create(poetry, config_settings)
activate Builder
Builder->>Builder: __init__()
Builder->>Builder: _apply_local_version_label()
alt has local version label
Builder->>Poetry: get package
Poetry->>Package: get version
Package-->>Builder: current version
Builder->>Package: replace(local=local_version_label)
end
deactivate Builder
Class diagram showing Builder class modificationsclassDiagram
class Builder {
+str|None format
-Poetry _poetry
-dict _config_settings
-Path _path
-set|None _excluded_files
+__init__(poetry: Poetry, executable: Path|None, config_settings: dict|None)
+build(target_dir: Path|None) Path
-_apply_local_version_label()
+executable() Path
+default_target_dir() Path
}
note for Builder "Added config_settings parameter and local version handling"
Flow diagram for build process with local versionflowchart TD
A[Start Build] --> B{Has config_settings?}
B -->|Yes| C{Has local-version?}
B -->|No| E[Continue Build]
C -->|Yes| D[Apply local version label]
C -->|No| E
D --> E
E --> F[Build package]
F --> G[End Build]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation for the new local-version config setting to help users understand how to use this feature.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
finswimmer
force-pushed
the
local-version-to-api
branch
from
25e0ccf to
90a5e9e
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation for the new local version label feature, including how to use it and examples of valid values for the
local-versionconfig setting.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
finswimmer
force-pushed
the
local-version-to-api
branch
from
90a5e9e to
fd81a1c
Compare
|
@finswimmer also see #715 |
fix: `update`, `add` and `remove` shall not uninstall extra dependencies With this change unrequested extras dependencies will also be kept when running `install` and are only removed when running `sync`! Fix`build` help regarding `--clean` (#9994)
finswimmer
force-pushed
the
local-version-to-api
branch
from
fd81a1c to
677b346
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation for the new local version label config setting feature. Since this implements a PEP 517 capability, it should be documented for users who want to utilize this functionality.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -72,6 +82,13 @@ def executable(self) -> Path: | |||
| def default_target_dir(self) -> Path: | |||
| return self._path / "dist" | |||
|
|
|||
| def _apply_local_version_label(self) -> None: | |||
There was a problem hiding this comment.
nit picky, but can we make this more generic so we can add other config settings in the future? apply_config_settings or something?
There was a problem hiding this comment.
I thought about it too, but came to the conclusion that the point when a config setting is applied depends on what the setting is for. So I decided for a specific method name for now.
There was a problem hiding this comment.
In that case why even bother with a method? why not just do that inline?
There was a problem hiding this comment.
In that case why even bother with a method? why not just do that inline?
Readability 😃 I don't want to mess up the init Method with logic.
There was a problem hiding this comment.
/shrug not sure if that adds much in terms of readability right now, in fact I feel it is the other way. That said, not going to block on this as it does what it says on the tin, we can always re-evaluate later.
|
@finswimmer since this is getting merged, can you please coordinate with the author of #715 to see what needs to be done there now. |
Adds the ability to pass a local version label to the build backend via
config_settingsas described in PEP 517Summary by Sourcery
Add the ability to pass a local version label to the build backend interface, allowing for more flexible versioning in builds. Update the Builder class to handle config_settings and apply local version labels to package versions. Include tests to ensure the new feature works as expected.
New Features:
Tests: