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

introduce sync command #9801

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

radoering
Copy link
Member

@radoering radoering commented Oct 27, 2024

Pull Request Check List

As suggested in #9136 (comment). Still not sure about it and how much duplication in the docs would be needed.

Closes #9855

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

@radoering radoering added the impact/docs Contains or requires documentation changes label Oct 27, 2024
Copy link

github-actions bot commented Oct 27, 2024

Deploy preview for website ready!

✅ Preview
https://website-a1uaufsh6-python-poetry.vercel.app

Built with commit 2812a1a.
This pull request is being automatically deployed with vercel-action

@trim21
Copy link
Contributor

trim21 commented Oct 27, 2024

is it possible adding this to v1, too?

@radoering
Copy link
Member Author

is it possible adding this to v1, too?

No, I do not think so. We will probably only do another 1.x release if there is a critical issue and we will not include new features in that case.

@abn
Copy link
Member

abn commented Nov 17, 2024

@radoering a point of note here; we have until now avoided or tried to avoid cases where we have 2 ways to do the same thing via the CLI. This definitely breaks that model (we actually might have already broken it). I do not have a strong opinion on this as long as the underlying code is clearly an alias and documented as such. Wanted to note this here and want to make sure this is intentional.

@radoering
Copy link
Member Author

as long as the underlying code is clearly an alias and documented as such.

Agreed.

Wanted to note this here and want to make sure this is intentional.

It stems from the wish of some users that --sync should be the default for install. On the one hand, I agree because I almost always use --sync myself and I think it is a good default for the majority of users. On the other hand, I do not think it works well with virtualenvs.create = false or virtualenvs.options.system-site-packages = true. See #9136 (comment) and the following discussion for details.

I added a note with some additional information to the docs.

@Secrus
Copy link
Member

Secrus commented Nov 17, 2024

as long as the underlying code is clearly an alias and documented as such.

I have to disagree. If this is to be just an alias, I don't see a reason to make this change. Splitting the behavior makes more sense to me. install just installs the project (and we remove --sync from it) and sync installs and clears the env from everything that is not part of the project. That gives us more flexibility for future features (like #3033) while keeping the "alias" would be forcing us to dance around that and make it more difficult to maintain in the long run.

@abn
Copy link
Member

abn commented Nov 18, 2024

@Secrus I do not disagree. My point, to be clearer is, if we are duplicating functionality I am not (personally) strongly opposed to a clearly documented alias. If we were splitting functionality, that is perfectly fine too, as long as we deprecate/remove the original. Whatever we choose, we just need to be clear on what and why.

On the other hand, I do not think it works well with virtualenvs.create = false or virtualenvs.options.system-site-packages = true.

@radoering If it were up-to me, I would just enforce virtual environments and be done with it. But I am sure some containers-don't-need-venv-footgun-warriors will be up in arms. I am surprised the system-site-packages case is impacted, as we should not be touching those packages at all. But then again, I guess detection is not always the easiest.

As I write this, I am weirdly becoming more opposed to the "sync" as a new command idea. I much rather have poetry install --sync as the default because not doing so is a bigger foot gun for majority of developers. I have had cases where I have myself had a stray package in my venv that allowed tests to pass and the CI failing because it was a transient dependency of another. Baby proofing the virtualenvs.create = false camp seems like a step in the wrong direction. It somehow feels like not doing sync as default for install is breaking the promise of the install install - at least philosophically.

All that said, I am open to see what the consensus between the rest of the maintainers is and go with it.

@radoering radoering requested a review from a team December 29, 2024 10:15
description = "Update the project's environment according to the lockfile."

options: ClassVar[list[Option]] = [
opt for opt in InstallCommand.options if opt.name != "sync"
Copy link
Member

Choose a reason for hiding this comment

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

Since we are introducing it as a command, maybe we could remove the --sync option from install? I don't see why we would keep a duplicate (especially with the major version bump).

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to avoid this breaking change because it would break a lot of CI scripts. (Most of our other breaking changes are not really relevant for most users or at least not relevant for CI.) But we can deprecate it.

Copy link
Member

Choose a reason for hiding this comment

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

ok, let's deprecate it, but with a set date (June 2025 sounds reasonable). I'd like us to avoid a long-living deprecation we are stuck with forever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, there is also poetry self install --sync.

Option a) Do not deprecate --sync for poetry self install (only for poetry install).
Option b) Introduce poetry self sync for the symmetry.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have chosen option b).

docs/cli.md Outdated
Comment on lines 277 to 279
The `sync` command makes sure that the project's environment is in sync with the `poetry.lock` file.
It is equivalent to running `poetry install --sync` and provides the same options
(except for `--sync`) as [install]({{< relref "#install" >}}).
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 avoid the direct reference to poetry install here. Explanation and an options list like for other commands, would be more open-ended and wouldn't be considered "breaking" if we decide to split install and sync even more in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I thought that too. I have just been too lazy so far, but I will make up for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should look better now.


# import all tests from the install command
# and run them for sync by overriding the command fixture
from tests.console.commands.test_install import * # noqa: F403
Copy link
Member

Choose a reason for hiding this comment

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

Just a nitpick to satisfy the mind goblins: I hate that we have to do that like this, but I don't see any better way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dito.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants