Add output for poetry lock --check by artemrys · Pull Request #5081 · python-poetry/poetry

artemrys · 2022-01-21T23:43:41Z

Pull Request Check List

Resolves: #5038

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

radoering · 2022-01-22T16:42:25Z

I don't like the wording "up to date" resp. "out of date" because it is kind of ambiguous. The command does not check if some dependencies in the lock file are not up to date (as long as they comply with the constraints from pyproject.toml). The command only checks if poetry.lock is consistent with pyproject.toml (see documentation). Maybe, we should stick with the wording from the documentation?

Further, I would say the minimum fix is running poetry lock --no-update (only making poetry.lock consistent with pyproject.toml not updating any dependencies already locked). Thus, I would propose to run poetry lock [--no-update] (square brackets for not required but sensible option).

I am aware that the warning when running poetry install says: Warning: The lock file is not up to date with the latest changes in pyproject.toml. You may be getting outdated dependencies. Run update to update them. I think this message is ok, because it defines up to date more precisely by adding with the latest changes in pyproject.toml. However, I think poetry lock [--no-update] would be a better recommendation in this case, too.

All in all, I think the wording should be harmonized. Either consistent with pyproject.toml or up to date with the latest changes in pyproject.toml. Further, I would propose to recommend running poetry lock [--no-update] instead of poetry update.

But that is only my opinion. Any strong opinions from @python-poetry/triage ?

radoering · 2022-01-22T16:45:24Z

src/poetry/console/commands/lock.py

+                self.line("poetry.lock is up to date")
+                return 0
+            self.line(
+                "Error: poetry.lock is out of date. Run `poetry update` to fix it"


You should use <error>...</error> as in

poetry/src/poetry/console/commands/check.py

Line 26 in 28d05d0

self.line(f"<error>Error: {error}</error>")

for coloring.

Should we wait on an opinion from triage team on this one?

I think adding <error>...</error> is safe.

Considering the wording: Since there are no replies yet, either nobody had time to think about it or has no strong opinion or is just fine with the proposal. 😉 If you want to speed up the process (and are not afraid from some more iterations of changes) we can push this PR until I will approve it. Before merging, a core member will look at the changes anyway and may (or may not) request further changes.

Thus, if you want to proceed, I propose to stick with the wording from the documentation (poetry.lock is (not) consistent with pyproject.toml) and recommend running poetry lock [--no-update]. Otherwise, you can also just wait some more days for additional opinions.

I propose to stick with the wording from the documentation (poetry.lock is (not) consistent with pyproject.toml) and recommend running poetry lock [--no-update]

I like this one 👍

Updated messages

radoering · 2022-02-01T04:37:36Z

Can you adapt the warnings in

poetry/src/poetry/installation/installer.py

Lines 261 to 264 in 08ed8ed

    
           "Warning: The lock file is not up to date with " 
        
           "the latest changes in pyproject.toml. " 
        
           "You may be getting outdated dependencies. " 
        
           "Run update to update them."

and

poetry/src/poetry/console/commands/export.py

Lines 62 to 65 in 08ed8ed

    
           "Warning: The lock file is not up to date with " 
        
           "the latest changes in pyproject.toml. " 
        
           "You may be getting outdated dependencies. " 
        
           "Run update to update them."

to the new wording, please?

I'd say we replace the first and the last sentence with the sentences from the error message and replace "outdated" in the second sentence by "improper" (or similar).

radoering · 2022-02-01T04:40:50Z

src/poetry/console/commands/lock.py

-                if self.poetry.locker.is_locked() and self.poetry.locker.is_fresh()
-                else 1
+            if self.poetry.locker.is_locked() and self.poetry.locker.is_fresh():
+                self.line("poetry.lock is consistent with pyproject.toml")


I would add a punctuation mark at the end of the message.

artemrys · 2022-02-02T23:33:49Z

I've updated installer's and export's messages to use new wording. I can add tests for that code in the next PR.

radoering · 2022-02-03T15:48:08Z

Sorry, for nitpicking again, but hopefully the last request for change from me. 😉 I didn't meant to change the warnings in installer and export to errors but only to change the wording. Due to the fact that the commands are not aborted, a warning seems to be more suitable than an error for these commands.

Just to be clear: The error for poetry check --lock is suitable.

artemrys · 2022-02-08T14:01:45Z

Sorry, for nitpicking again, but hopefully the last request for change from me. 😉 I didn't meant to change the warnings in installer and export to errors but only to change the wording. Due to the fact that the commands are not aborted, a warning seems to be more suitable than an error for these commands.

Just to be clear: The error for poetry check --lock is suitable.

Done :)

radoering · 2022-02-08T16:08:57Z

src/poetry/console/commands/export.py


        if not locker.is_fresh():
-            self.line_error(
+            self._io.write_line(


Probably, line_error() is correct here, because warnings should go to stderr instead of stdout. I know, in installer.py the warning is written to stdout... Maybe, something that can be harmonized for all warnings in a separate PR...

radoering · 2022-02-08T16:09:15Z

src/poetry/console/commands/export.py

-                "the latest changes in pyproject.toml. "
-                "You may be getting outdated dependencies. "
-                "Run update to update them."
+                "Error: poetry.lock is not consistent with pyproject.toml. "


This should start with "Warning: "

radoering · 2022-02-08T16:09:35Z

src/poetry/installation/installer.py

-                    "the latest changes in pyproject.toml. "
-                    "You may be getting outdated dependencies. "
-                    "Run update to update them."
+                    "Error: poetry.lock is not consistent with pyproject.toml. "


This should also start with "Warning: "

artemrys · 2022-02-08T16:41:18Z

Sorry, it should have taken less time to figure this out for me :)

radoering

Sorry, I missed this before.

radoering · 2022-02-08T16:58:37Z

src/poetry/console/commands/export.py

-                "Run update to update them."
+                "Warning: poetry.lock is not consistent with pyproject.toml. "
+                "You may be getting improper dependencies. "
+                "Run `poetry update` to fix it."


Should be poetry lock [--no-update] instead of poetry update.

Same in installer.py.

radoering · 2022-02-08T18:54:53Z

Finally looks good to me.

@finswimmer : Can you approve running the workflows and consider merging if the tests pass?

github-actions · 2024-02-29T18:08:54Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Add output for poetry lock --check

67a00d1

artemrys marked this pull request as ready for review January 22, 2022 10:58

radoering requested changes Jan 22, 2022

View reviewed changes

Rephrase messages

2cc1a3b

radoering reviewed Feb 1, 2022

View reviewed changes

artemrys added 2 commits February 3, 2022 00:32

Add punctuation mark at the end of the message

1f9f3d4

Update wording for installer and export

cd9eaa5

Put warning back for installer and export

456359a

radoering reviewed Feb 8, 2022

View reviewed changes

Finalize warning messages

b1e4b57

radoering reviewed Feb 8, 2022

View reviewed changes

Improve fix messages

94eeecc

radoering approved these changes Feb 8, 2022

View reviewed changes

finswimmer approved these changes Feb 9, 2022

View reviewed changes

finswimmer merged commit 1196923 into python-poetry:master Feb 9, 2022

artemrys deleted the poetry-lock-messages branch February 9, 2022 07:57

finswimmer mentioned this pull request Mar 6, 2022

Release 1.2.0b1 #5280

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024

Comments

Conversation

artemrys commented Jan 21, 2022

Pull Request Check List

Uh oh!

radoering commented Jan 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemrys Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemrys Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radoering commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemrys commented Feb 2, 2022

Uh oh!

radoering commented Feb 3, 2022

Uh oh!

artemrys commented Feb 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemrys commented Feb 8, 2022

Uh oh!

radoering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radoering commented Feb 8, 2022

Uh oh!

github-actions bot commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

artemrys Jan 26, 2022 •

edited

Loading

artemrys Jan 31, 2022 •

edited

Loading