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 1 commit 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-aql5nrb0r-python-poetry.vercel.app

Built with commit d6b43ff.
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 marked this pull request as ready for review December 14, 2024 09:52
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