make deprecations of tool.poetry fields more prominent
#900
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR changes validation so that deprecation warnings for legacy [tool.poetry] fields are always emitted (not only in strict mode), updates tests/fixtures to reflect the new behavior and modern [project] usage, and slightly refactors validation ordering in Factory.validate to always run legacy-vs-project checks. Sequence diagram for validation flow with always-on legacy field warningssequenceDiagram
actor User
participant CLI as PoetryCLI
participant Factory as Factory
participant ValidatorResult as ValidationResult
User->>CLI: run_command()
CLI->>Factory: validate(toml_data, strict)
Factory->>ValidatorResult: create_or_init(result)
Factory->>Factory: _validate_dependency_groups(toml_data, result)
Factory->>Factory: _validate_legacy_vs_project(toml_data, result)
alt strict is True
Factory->>Factory: _validate_project(project, result)
Factory->>Factory: _validate_strict(config, result)
end
Factory-->>CLI: result (with legacy field warnings if present)
CLI-->>User: display warnings and errors
Class diagram for Factory validation and legacy vs project checksclassDiagram
class Factory {
+validate(toml_data, strict) ValidationResult
+_validate_dependency_groups(toml_data, result) None
+_validate_legacy_vs_project(toml_data, result) None
+_validate_project(project, result) None
+_validate_strict(config, result) None
}
class ValidationResult {
+errors list
+warnings list
+add_error(message) None
+add_warning(message) None
}
Factory ..> ValidationResult : uses
Factory ..> ValidationResult : populate_errors_and_warnings
Factory ..> Factory : calls_validate_dependency_groups
Factory ..> Factory : calls_validate_legacy_vs_project
Factory ..> Factory : calls_validate_project_in_strict_mode
Factory ..> Factory : calls_validate_strict_in_strict_mode
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 there - I've reviewed your changes - here's some feedback:
- By moving
_validate_legacy_vs_projectout of the strict branch you are now always running all legacy/project consistency checks, including any hard errors they may emit; if the goal is to only surface deprecation warnings fortool.poetryfields in non-strict mode, consider either splitting warning-only checks from error checks or scoping what runs outsidestrict. - In
tests/masonry/builders/fixtures/with_bad_path_dep/pyproject.tomlthe new[project]metadata usespython-requiresrather than the PEP 621requires-pythonkey, and you removed the previous[tool.poetry.dependencies].pythonentry; double-check that this fixture still models the intended Python constraint and doesn’t accidentally change what the test is validating.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By moving `_validate_legacy_vs_project` out of the strict branch you are now always running all legacy/project consistency checks, including any hard errors they may emit; if the goal is to only surface deprecation warnings for `tool.poetry` fields in non-strict mode, consider either splitting warning-only checks from error checks or scoping what runs outside `strict`.
- In `tests/masonry/builders/fixtures/with_bad_path_dep/pyproject.toml` the new `[project]` metadata uses `python-requires` rather than the PEP 621 `requires-python` key, and you removed the previous `[tool.poetry.dependencies].python` entry; double-check that this fixture still models the intended Python constraint and doesn’t accidentally change what the test is validating.
## Individual Comments
### Comment 1
<location> `tests/test_factory.py:722-731` </location>
<code_context>
-
-
-def test_validate_strict_legacy_warnings(complete_legacy_warnings: list[str]) -> None:
[email protected]("strict", [False, True])
+def test_validate_strict_legacy_warnings(
+ complete_legacy_warnings: list[str], strict: bool
+) -> None:
complete = fixtures_dir / "complete.toml"
with complete.open("rb") as f:
content = tomllib.load(f)
- assert Factory.validate(content, strict=True) == {
+ assert Factory.validate(content, strict=strict) == {
"errors": [],
"warnings": complete_legacy_warnings,
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test case for a config without any legacy fields to ensure no deprecation warnings are emitted in either strict or non-strict mode.
To fully cover the new behavior, please add a complementary test using a config without any legacy `tool.poetry` fields and assert that `Factory.validate(..., strict=False)` and `Factory.validate(..., strict=True)` both return an empty `warnings` list. This helps guard against regressions where `_validate_legacy_vs_project` might start emitting warnings for valid `project`-only configurations.
Suggested implementation:
```python
assert Factory.validate(content, strict=strict) == {
"errors": [],
"warnings": complete_legacy_warnings,
}
assert Factory.validate(content) == {"errors": [], "warnings": []}
@pytest.mark.parametrize("strict", [False, True])
def test_validate_project_only_no_legacy_warnings(strict: bool) -> None:
project_only = fixtures_dir / "project_only.toml"
with project_only.open("rb") as f:
content = tomllib.load(f)
assert Factory.validate(content, strict=strict) == {
"errors": [],
"warnings": [],
}
def test_validate_fails(complete_legacy_warnings: list[str]) -> None:
complete = fixtures_dir / "complete.toml"
with complete.open("rb") as f:
content = tomllib.load(f)
expected = "tool.poetry.authors must be array"
```
1. Ensure that a fixture file `project_only.toml` exists under `fixtures_dir` and that it represents a valid configuration using only `project`-style metadata, with no legacy `tool.poetry` metadata fields.
2. If `fixtures_dir` uses a different naming convention for such a fixture (e.g. `project_only_pyproject.toml` or similar), update the filename in `project_only = fixtures_dir / "project_only.toml"` accordingly.
3. Optionally, if the default behavior (`Factory.validate(content)`) is meant to be tested for the project-only config as well, you can add an additional assertion mirroring `assert Factory.validate(content) == {"errors": [], "warnings": []}` inside the new test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+722
to
731
| @pytest.mark.parametrize("strict", [False, True]) | ||
| def test_validate_strict_legacy_warnings( | ||
| complete_legacy_warnings: list[str], strict: bool | ||
| ) -> None: | ||
| complete = fixtures_dir / "complete.toml" | ||
| with complete.open("rb") as f: | ||
| content = tomllib.load(f) | ||
|
|
||
| assert Factory.validate(content, strict=True) == { | ||
| assert Factory.validate(content, strict=strict) == { | ||
| "errors": [], |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for a config without any legacy fields to ensure no deprecation warnings are emitted in either strict or non-strict mode.
To fully cover the new behavior, please add a complementary test using a config without any legacy tool.poetry fields and assert that Factory.validate(..., strict=False) and Factory.validate(..., strict=True) both return an empty warnings list. This helps guard against regressions where _validate_legacy_vs_project might start emitting warnings for valid project-only configurations.
Suggested implementation:
assert Factory.validate(content, strict=strict) == {
"errors": [],
"warnings": complete_legacy_warnings,
}
assert Factory.validate(content) == {"errors": [], "warnings": []}
@pytest.mark.parametrize("strict", [False, True])
def test_validate_project_only_no_legacy_warnings(strict: bool) -> None:
project_only = fixtures_dir / "project_only.toml"
with project_only.open("rb") as f:
content = tomllib.load(f)
assert Factory.validate(content, strict=strict) == {
"errors": [],
"warnings": [],
}
def test_validate_fails(complete_legacy_warnings: list[str]) -> None:
complete = fixtures_dir / "complete.toml"
with complete.open("rb") as f:
content = tomllib.load(f)
expected = "tool.poetry.authors must be array"- Ensure that a fixture file
project_only.tomlexists underfixtures_dirand that it represents a valid configuration using onlyproject-style metadata, with no legacytool.poetrymetadata fields. - If
fixtures_diruses a different naming convention for such a fixture (e.g.project_only_pyproject.tomlor similar), update the filename inproject_only = fixtures_dir / "project_only.toml"accordingly. - Optionally, if the default behavior (
Factory.validate(content)) is meant to be tested for the project-only config as well, you can add an additional assertion mirroringassert Factory.validate(content) == {"errors": [], "warnings": []}inside the new test.
Member
|
I wonder if we could add a way for users to disable the warnings (either via config or env var). I know some people just don't want to migrate because of "reasons" and they should be able to set |
Member
Author
|
A config setting had to live in poetry. poetry-core has no config. It is easy to introduce an env var. However, I do not like to thoughlessly introduce env vars without a config setting. (It is easy to lose track of them.)
Is this about the project section in general or just about dependencies? Do we want to care about those people? (It may be easier to answer that question if we knew the "reasons".) Then, we may just not make the deprecations more prominent and keep them as is until we think we do not have to care anymore. I am perfectly fine with that. |
Member
I agree, however I would still add an opt-out env var (we can mark it with a FIXME or TODO in the code. Just to avoid people complaining in issues/discussions.
The
Personally, I am all for breaking fast and often with clear migration paths, so I would say "no". |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Almost one year and three minor versions after Poetry 2.0, I think we should make the warnings for the deprecated
tool.poetryfields more prominent. Currently, they are only returned in strict validation mode, i.e. when runningpoetry check. With this PR, they are always returned, i.e. they are printed when any Poetry command is run.I think this makes sense if we ever want to remove the deprecated fields - even in a major release.
Summary by Sourcery
Always validate and surface deprecated [tool.poetry] field warnings, not only in strict mode, and adjust validation behavior and tests accordingly.
Bug Fixes:
Enhancements:
Tests: