Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add output for poetry lock --check #5081

Merged
merged 7 commits into from
Feb 9, 2022
Merged

Add output for poetry lock --check #5081

merged 7 commits into from
Feb 9, 2022

Conversation

artemrys
Copy link
Contributor

Pull Request Check List

Resolves: #5038

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

@artemrys artemrys marked this pull request as ready for review January 22, 2022 10:58
@radoering
Copy link
Member

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 ?

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

for coloring.

Copy link
Contributor Author

@artemrys artemrys Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor Author

@artemrys artemrys Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated messages

@radoering
Copy link
Member

Can you adapt the warnings in

"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
"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).

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@artemrys
Copy link
Contributor Author

artemrys commented Feb 2, 2022

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
Copy link
Member

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
Copy link
Contributor Author

artemrys commented Feb 8, 2022

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 :)

@@ -57,12 +57,11 @@ def handle(self) -> None:
self.call("lock", " ".join(options))

if not locker.is_fresh():
self.line_error(
self._io.write_line(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

"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. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should start with "Warning: "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

"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. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also start with "Warning: "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@artemrys
Copy link
Contributor Author

artemrys commented Feb 8, 2022

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

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this before.

"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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same in installer.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@radoering
Copy link
Member

Finally looks good to me.

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

@finswimmer finswimmer merged commit 1196923 into python-poetry:master Feb 9, 2022
@artemrys artemrys deleted the poetry-lock-messages branch February 9, 2022 07:57
@finswimmer finswimmer mentioned this pull request Mar 6, 2022
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poetry lock --check has no output
3 participants