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

Remove unnecessary explicit use of 'cwd' #9113

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

Conversation

Cypher1
Copy link
Contributor

@Cypher1 Cypher1 commented Mar 5, 2024

This removes an explicit override of the cwd argument to Factory.create_poetry which already computes the current working directory.

This de-risks changes currently being considered python-poetry/poetry-core#695 and avoids a runtime import of pathlib.

…' behavior rather than just looking at the current working directory
@abn
Copy link
Member

abn commented Mar 5, 2024

I don't think this change is required. This makes things less explicit. And relies on implicit behavior.

And as mentioned in the referenced PR, that change might not be accepted.

@Cypher1
Copy link
Contributor Author

Cypher1 commented Mar 5, 2024

@abn I agree that it is less explicit, but the current implementation is explicitly misleading.

In the current implementation the directory used is not the current working directory, but ANY directory including or above the current working directory containing a pyproject.toml.

This has bitten me and probably others multiple times when using poetry in projects that have subprojects. I'd like to address this and this is one of the instances where the assumption is baked in. This should help poetry support monorepos in future, but for now is just updating the code to match the documentation.

@abn
Copy link
Member

abn commented Mar 5, 2024

In the current implementation the directory used is not the current working directory, but ANY directory including or above the current working directory containing a pyproject.toml.

This is by design. Similar to git.

This has bitten me and probably others multiple times when using poetry in projects that have subprojects. I'd like to address this and this is one of the instances where the assumption is baked in. This should help poetry support monorepos in future, but for now is just updating the code to match the documentation.

I am not sure I quite follow. Can you clarify how your change makes things better? If you actually have a subproject, your current directory pyproject should be picked up, if not, the direct parents are searched. I might be missing something obvious here, so please feel free to let me know.

@eamanu
Copy link
Contributor

eamanu commented Mar 9, 2024

IMO this change is not necessary right now, I think is better be explicit.

This should help poetry support monorepos in future, but for now is just updating the code to match the documentation.

But maybe would be better introduce this change with a feature to be better look the benefits of remove the explicit cwd.

@Cypher1
Copy link
Contributor Author

Cypher1 commented Mar 29, 2024

In the current implementation the directory used is not the current working directory, but ANY directory including or above the current working directory containing a pyproject.toml.

This is by design. Similar to git.

I think you misunderstand me.

I totally support the default behaviour matching git as you say. I'm specifically describing the behaviour when an explicit -C argument is passed in.

This behaviour leads to poetry running from parent directories that are not described by the -C argument when the -C argument is not an existing directory.

My belief is that poetry should (at minimum) warn the user when the user has requested a project directory that does not exist, and preferrably should not run the command (which may be destructive to a project that the user did not intend to modify).

Thanks for reconsidering this and for asking further when I was not sufficiently clear earlier.

@Secrus Secrus added this to the Poetry 2.0 milestone Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants