fix: Fix handling of "format" in package includes initialization #805
Conversation
Reviewer's Guide by SourceryThis pull request fixes the handling of the "format" key in package includes initialization by ensuring that "format" is always processed as a list. If the "format" key is not explicitly set, it defaults to ["sdist", "wheel"] for packages and ["sdist"] for includes. Sequence diagram for package format initializationsequenceDiagram
participant Factory as Poetry Factory
participant Package as Project Package
Factory->>Factory: _prepare_formats(items, default_formats)
Note over Factory: Process format as list
Factory->>Factory: _configure_package_poetry_specifics()
Factory->>Package: include = prepare_formats(includes, ['sdist'])
Factory->>Package: packages = prepare_formats(packages, ['sdist', 'wheel'])
Class diagram showing Factory and Package relationshipclassDiagram
class Factory {
+_prepare_formats(items: list, default_formats: list)
+_configure_package_poetry_specifics(package: ProjectPackage, tool_poetry: dict)
}
class ProjectPackage {
+include: list
+packages: list
+build_config: dict
+exclude: list
}
Factory ..> ProjectPackage: configures
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
A test would be in order |
de0d617 to
d158c7b
Compare
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please update the documentation to reflect the format parameter behavior and default values, as indicated in the PR checklist.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
tests/masonry/utils/test_module.py
Outdated
| name=package.name, directory=str(directory), packages=package.packages | ||
| ) | ||
| assert len(module.includes) == 1 | ||
| assert module.includes[0].formats == expected_formats |
There was a problem hiding this comment.
suggestion (testing): Add a test case for an invalid format.
It would be beneficial to add a test case that uses an invalid format value (e.g., an integer, a dictionary, or an invalid string) to ensure that the code handles such scenarios gracefully and raises an appropriate error.
Suggested implementation:
package = poetry.package
assert "format" not in package.packages[0]
if given_format:
package.packages[0]["format"] = given_format # type: ignore[index]
module = Module(
name=package.name, directory=str(directory), packages=package.packages
)
assert len(module.includes) == 1
assert module.includes[0].formats == expected_formats
def test_module_with_invalid_format(poetry):
directory = poetry.file.parent
package = poetry.package
# Test with an integer format (invalid)
package.packages[0]["format"] = 42 # type: ignore[index]
with pytest.raises(ValueError) as exc_info:
Module(
name=package.name, directory=str(directory), packages=package.packages
)
assert "Invalid format value" in str(exc_info.value)
# Test with a dictionary format (invalid)
package.packages[0]["format"] = {"key": "value"} # type: ignore[index]
with pytest.raises(ValueError) as exc_info:
Module(
name=package.name, directory=str(directory), packages=package.packages
)
assert "Invalid format value" in str(exc_info.value)
Note that this test assumes that the Module class already has validation that raises ValueError for invalid formats. If it doesn't, you'll need to:
- Add format validation in the Module class
- Adjust the error message assertions to match the actual error messages used in the Module class
- Import pytest if it's not already imported at the top of the file
| @@ -72,11 +72,15 @@ def __init__( | |||
| packages = [default_package] | |||
|
|
|||
| for package in packages: | |||
| formats = package.get("format", ["sdist", "wheel"]) | |||
There was a problem hiding this comment.
wasn't the point of #773 to default this only to "sdist"?
There was a problem hiding this comment.
In my understanding this was about the behavior of format for the include keyword and not for packages. 🤷I guess @radoering can say more about it.
There was a problem hiding this comment.
can confirm that I find this confusing
so I see, #773 had Builder._module() patch up the package and include objects to add default values if not present - but there is nothing corresponding to this in poetry's RunCommand._module().
Perhaps a better way would be to kill both birds with one stone and do this defaulting at the time the package definition is read, I think in poetry-core's Factory._configure_package_poetry_specifics. Then it is always available for everyone and no further special casing is needed.
There was a problem hiding this comment.
Sounds better than having two spots for the default handling.
There was a problem hiding this comment.
Ah, I guess you found the places I was looking for initially. I'm on mobile so I have to take a look at later/tomorrow.
There was a problem hiding this comment.
Maybe, the fix should be downstream. Give me some time to think about it. Currently, all the default handling is in
poetry-core/src/poetry/core/masonry/builders/builder.py
Lines 49 to 65 in 61081dd
poetry run. We have different defaults for package includes and include includes. See python-poetry/poetry#9691 for details.
d158c7b to
6974e7a
Compare
|
I am wondering if the downstream tests show some unwanted behavior. I remember these tests were the reason I did not do this in the factory but I do not remember if it is a real issue or just the tests... It might be that we are producing unwanted changes in the |
6974e7a to
4a63f82
Compare
My previous implementation was problematic because it changes the data read from the toml directly. I changed it, so that the toml data stays untouched and only the data that goes into the The only downstream test that is failing, is the same, that we need to adopt in |
|
Looks good now.
Also not sure. It might make sense because downstream we inherit from Can you prepare a downstream PR? (I think we can merge this with the failing test but we should update downstream to use poetry-core's master branch shortly afterwards so that we keep the time of failing downstream tests short.) |
|
One more thing: Can we move the default handling completely into poetry-core/src/poetry/core/masonry/builders/builder.py Lines 49 to 65 in 61081dd |
95b75d0 to
7bfad90
Compare
Ensure that "format" is always processed as a list when initializing package includes. If not explicit set default to sdist and wheel.
7bfad90 to
0976e7e
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 update the documentation to describe the default format values (sdist+wheel for packages, sdist for includes) and that the format field should be a list.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues 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.
| ] | ||
|
|
||
| assert package.include == [ | ||
| {"path": "extra_dir/vcs_excluded.py", "format": ["sdist", "wheel"]}, | ||
| {"path": "notes.txt"}, | ||
| {"path": "notes.txt", "format": ["sdist"]}, |
There was a problem hiding this comment.
suggestion (testing): Test explicit format exclusion
Add a test case where "format" explicitly excludes a default format (e.g., format: [] or format: ['wheel'] for a package include) to verify that the package is correctly excluded from the specified format.
Suggested implementation:
assert package.include == [
{"path": "extra_dir/vcs_excluded.py", "format": ["sdist", "wheel"]},
{"path": "notes.txt", "format": ["sdist"]},
{"path": "format_excluded.txt", "format": ["wheel"]},
]You'll also need to:
- Create a 'format_excluded.txt' file in the test directory structure
- Add the corresponding entry in the test setup/fixture that creates this test file
- Update any related test data that defines the package includes to include this new file
I prepared one here: python-poetry/poetry#9992
Done. (At least I think so 😀 ) |
0976e7e to
977ec56
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 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.
977ec56 to
c62629c
Compare
Ensure that "format" is always processed as a list when initializing package includes. If not explicit set default to sdist and wheel.
Resolves: python-poetry/poetry#9961
Summary by Sourcery
Bug Fixes:
Summary by Sourcery
Bug Fixes: