fix(console): do not fail on warnings on check command #9983

finswimmer · 2025-01-07T11:51:32Z

Pull Request Check List

Old behavior is retained by new parameter --strict.

Resolves: #9971

Added tests for changed code.
Updated documentation for changed code.

Summary by Sourcery

Add the --strict option to the check command to make it fail if warnings are reported. By default, the command will only report warnings and exit with a success code.

New Features:

Introduce the --strict option to the poetry check command.

Tests:

Add tests for the --strict option.

sourcery-ai · 2025-01-07T11:51:37Z

Reviewer's Guide by Sourcery

This pull request introduces a new --strict option to the check command. This option allows the command to fail if warnings are reported. The old behavior is retained by default, meaning warnings are reported but do not cause the command to fail unless the --strict option is used.

Sequence diagram for Poetry check command behavior

sequenceDiagram
    actor User
    participant CLI
    participant CheckCommand

    User->>CLI: poetry check [--strict]
    CLI->>CheckCommand: handle()
    CheckCommand->>CheckCommand: validate and check
    Note over CheckCommand: Collect errors and warnings
    alt has errors OR (has warnings AND strict mode)
        CheckCommand-->>CLI: return code 1
        CLI-->>User: Show errors/warnings & exit 1
    else no errors AND (no warnings OR not strict)
        CheckCommand-->>CLI: return code 0
        CLI-->>User: Show "All set!" & exit 0
    end

State diagram for check command return status

stateDiagram-v2
    [*] --> Checking
    Checking --> HasErrors: Errors found
    Checking --> HasWarnings: Only warnings found
    Checking --> Success: No issues

    HasErrors --> Failure: return 1
    HasWarnings --> StrictMode: --strict
    HasWarnings --> Success: no --strict
    StrictMode --> Failure: return 1
    Success --> [*]: return 0
    Failure --> [*]

File-Level Changes

Change	Details	Files
Added `--strict` option to `poetry check` command	The `check` command now accepts a `--strict` option. When the `--strict` option is provided, the command will exit with a non-zero status code if warnings are present. Tests were added to verify the behavior of the new `--strict` option. Documentation was updated to describe the new `--strict` option.	`src/poetry/console/commands/check.py` `tests/console/commands/test_check.py` `docs/cli.md`

Possibly linked issues

poetry check: distinguish warnings from errors? #9971: The PR fixes the issue by adding a --strict option to the check command, which makes it exit with error code 1 if there are warnings.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @finswimmer - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 2 issues found
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2025-01-07T11:52:27Z

src/poetry/console/commands/check.py


-            return 0
+        if return_code == 0:


issue: The 'All set!' message might be misleading when warnings are present

Consider only showing 'All set!' when there are no errors AND no warnings, regardless of the no-strict setting. Currently, it will show 'All set!' followed by warnings when no-strict is enabled.

tests/console/commands/test_check.py

docs/cli.md

dimbleby · 2025-01-07T11:59:30Z

I'm inclined to agree with the camp that thinks that warnings should not have resulted in exit code 1 in the first place. Warnings are only warnings, only errors are errors.

so I'd vote that way rather than throw more flags at it

Perhaps a --strict flag would make sense, so that people can choose to error on warnings, but I think the current default is the wrong way round

finswimmer · 2025-01-07T12:04:11Z

Perhaps a --strict flag would make sense, so that people can choose to error on warnings, but I think the current default is the wrong way round

I agree with you. But @Secrus and I had a short conversation about it on discord. Changing the current default, would be a breaking change. Of course one could argue, the current behavior is a bug and the change is only a fix ... Would like to hear opinions from the other maintainers.

dimbleby · 2025-01-07T12:08:33Z

"breaking" only in rather a weak sense of not failing where it could have done. I doubt that this will actually break anything for anyone, though it might cause them to not notice warnings.

imo 2.0 is sufficiently recent and the current behaviour is sufficiently unpopular that it is ok to rip the bandage and do what we think is right. Having to wait for poetry 3.0 to address this paper cut of backwards defaults seems worse in the long run.

(unless the team anticipates more regular major releases and 3.0 is relatively close...)

jeremiah-snee-openx · 2025-01-07T15:14:41Z

Current behavior definitely feels more like a bug. There was no transition period with warnings from poetry. This has resulted in breaking many production CI pipelines.

there is a command, poetry config --migrate, to migrate outdated configs, for when "If Poetry renames or remove config options...", but these changes are not updated by this command.

If the intention is for poetry check to throw exit 1, then these warnings should just be thrown as errors.

imo it is bad form to suddenly have an exit 1 for warnings. Especially with no prior warning to users.

dimbleby · 2025-01-07T15:21:11Z

suddenly have an exit 1 for warnings.

in fact warnings from poetry check were associated with non-zero exit code since forever. The change here is the new warnings.

abn · 2025-01-07T15:57:01Z

Just to add my voice to the conversation, I tend to agree that current behavior is a bug. We should not fail on warnings.

It could be considered a breaking change if we are to assume that the current behavior is intentional. It really is not. I am also cautiously optimistic that the change in behavior will not break anyone's already passing CI environments as one would assume they are already in-effect passing on a strict check.

Further, historically we only really consider cli interference changes as breaking change candidates.

My vote here is to change behavior to not fail on warnings and ass a --strict flag.

radoering · 2025-01-07T16:24:00Z

Pedantically speaking, it feels like a breaking change. However, I agree that in the long run, it is better to consider it as bugfix and do it the way it feels right (--strict).

finswimmer · 2025-01-07T16:44:35Z

Ok, I changed the behavior.

src/poetry/console/commands/check.py

abn · 2025-01-07T16:58:12Z

@sourcery-ai review

sourcery-ai

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: 1 issue found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tests/console/commands/test_check.py

jeremiah-snee-openx

LGTM

Old behavior is retained by new parameter `--strict`.

sourcery-ai bot reviewed Jan 7, 2025

View reviewed changes

finswimmer requested a review from a team January 7, 2025 11:55

This was referenced Jan 7, 2025

poetry check --lock has exit code 1 in Poetry 2 when used with Poetry 1 pyproject.toml #9958

Closed

poetry 2.0 check deprecation warnings producing errors when fixed #9965

Closed

finswimmer force-pushed the check-no-strict branch from 2bf2115 to 1db972f Compare January 7, 2025 16:43

finswimmer changed the title ~~feat(console): Add --no-strict option to check command for warnings handling~~ fix(console): do not fail on warnings on check command Jan 7, 2025

finswimmer force-pushed the check-no-strict branch from 1db972f to 5cd312c Compare January 7, 2025 16:45

radoering reviewed Jan 7, 2025

View reviewed changes

src/poetry/console/commands/check.py Show resolved Hide resolved

sourcery-ai bot reviewed Jan 7, 2025

View reviewed changes

tests/console/commands/test_check.py Show resolved Hide resolved

jeremiah-snee-openx approved these changes Jan 7, 2025

View reviewed changes

finswimmer added 2 commits January 7, 2025 18:31

fix(console): do not fail on warnings on check

eb047a5

Old behavior is retained by new parameter `--strict`.

docs: Add --strict option to CLI documentation

3bfa4cb

finswimmer force-pushed the check-no-strict branch from 5cd312c to 3bfa4cb Compare January 7, 2025 17:33

abn approved these changes Jan 7, 2025

View reviewed changes

abn merged commit d3b6ba0 into python-poetry:main Jan 7, 2025
74 checks passed

finswimmer mentioned this pull request Jan 8, 2025

"poetry check" errors but only show warnings #9991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(console): do not fail on warnings on check command #9983

fix(console): do not fail on warnings on check command #9983

finswimmer commented Jan 7, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jan 7, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

sourcery-ai bot Jan 7, 2025

dimbleby commented Jan 7, 2025 •

edited

Loading

finswimmer commented Jan 7, 2025

dimbleby commented Jan 7, 2025

jeremiah-snee-openx commented Jan 7, 2025

dimbleby commented Jan 7, 2025

abn commented Jan 7, 2025

radoering commented Jan 7, 2025

finswimmer commented Jan 7, 2025

abn commented Jan 7, 2025

sourcery-ai bot left a comment

jeremiah-snee-openx left a comment

fix(console): do not fail on warnings on check command #9983

fix(console): do not fail on warnings on check command #9983

Conversation

finswimmer commented Jan 7, 2025 • edited by sourcery-ai bot Loading

Pull Request Check List

Summary by Sourcery

sourcery-ai bot commented Jan 7, 2025 • edited Loading

Reviewer's Guide by Sourcery

Sequence diagram for Poetry check command behavior

State diagram for check command return status

File-Level Changes

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Jan 7, 2025

Choose a reason for hiding this comment

dimbleby commented Jan 7, 2025 • edited Loading

finswimmer commented Jan 7, 2025

dimbleby commented Jan 7, 2025

jeremiah-snee-openx commented Jan 7, 2025

dimbleby commented Jan 7, 2025

abn commented Jan 7, 2025

radoering commented Jan 7, 2025

finswimmer commented Jan 7, 2025

abn commented Jan 7, 2025

sourcery-ai bot left a comment

Choose a reason for hiding this comment

jeremiah-snee-openx left a comment

Choose a reason for hiding this comment

finswimmer commented Jan 7, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jan 7, 2025 •

edited

Loading

dimbleby commented Jan 7, 2025 •

edited

Loading