-
Notifications
You must be signed in to change notification settings - Fork 247
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
Allow non-existant path dependencies #507
Allow non-existant path dependencies #507
Conversation
for more information, see https://pre-commit.ci
I looked into the version history for this file to see if I could find some rational for the way it currently works. This goes all the way back to the "Initial commit"
|
…iangb/poetry-core into allow-non-existant-path-deps
@radoering any thoughts on this? |
I think it makes sense, but I have to take a deeper look if python-poetry/poetry#6844 is sufficient (i.e. covers all commands). For example, does Further, I think we should do a similar change for file dependencies because it's the same use case. |
related: python-poetry/poetry#668, #210 |
return | ||
|
||
if self._full_path.is_file(): | ||
raise ValueError(f"{self._path} is a file, expected a directory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either all checks should raise an error or all checks should emit a warning (different modes). If I don't care if the directory exists, I won't care if it has been replaced with a file.
raise ValueError(f"{self._path} is a file, expected a directory") | ||
|
||
if not is_python_project(self._full_path): | ||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. If I don't care for existence, I won't care if it's not a Python package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to push back on this a bit. There's a real use case for creating a DirectoryDependency
pointing to a path that does not exist: you deleted or renamed the dependency and are updating the lock file. I can't think of any use case where you'd want to point at an existing but invalid dependency, so in those cases I think failing fast is the best option.
try: | ||
self._full_path = self._base.joinpath(self._path).resolve() | ||
except FileNotFoundError: | ||
warn(f"Directory {self._path} does not exist", category=UserWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like mentioning the name of the dependency (how you did in the companion PR). Maybe, we can add it in the warning/error messages here, too.
# cache this function to avoid multiple IO reads and parsing | ||
self.supports_poetry = functools.lru_cache(maxsize=1)(self._supports_poetry) | ||
|
||
def _validate(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try the approach mentioned in #210 (comment). We need two modes for _validate
: a warning mode and an exception mode. The warning mode can be used in the __init__
and the exception mode in full_path
. I think it should be save to assume that when accessing full_path
the directory must exist (and be a valid project).
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
def test_create_poetry_fails_with_invalid_dev_dependencies_iff_with_dev_is_true() -> ( | ||
None | ||
): | ||
with pytest.raises(ValueError) as err: | ||
Factory().create_poetry(fixtures_dir / "project_with_invalid_dev_deps") | ||
assert "does not exist" in str(err.value) | ||
|
||
Factory().create_poetry( | ||
fixtures_dir / "project_with_invalid_dev_deps", with_groups=False | ||
) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this test since we actually want to be able to create a Poetry object with invalid path deps, we just want it to fail for certain operations (install, show, etc.).
@@ -22,7 +22,7 @@ | |||
|
|||
def test_file_dependency_wrong_path() -> None: | |||
with pytest.raises(ValueError): | |||
FileDependency("demo", DIST_PATH / "demo-0.2.0.tar.gz") | |||
FileDependency("demo", DIST_PATH / "demo-0.2.0.tar.gz").full_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests should be testing each individual failure mode, but this was good enough for years so I suppose it will do
extras=extras, | ||
) | ||
|
||
def _get_full_path(self, raise_if_invalid: bool, name: str) -> Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding name
was necessary because .name
doesn't get set until super().__init__
is called which already requires full_path
.
if self._full_path is not None: | ||
return self._full_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This little bit of caching means that if we already checked in __init__
we don't need to check again. We could also cache the negative case but it doesn't seem worth the complexity IMO.
raise ValueError(f"{self._path} is a file, expected a directory") | ||
|
||
if not is_python_project(self._full_path): | ||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to push back on this a bit. There's a real use case for creating a DirectoryDependency
pointing to a path that does not exist: you deleted or renamed the dependency and are updating the lock file. I can't think of any use case where you'd want to point at an existing but invalid dependency, so in those cases I think failing fast is the best option.
So this works. Conceptually though I don't like the idea of having |
You have got a point there. On the one hand it's an easier transition from "validation in |
Sounds good. Do you think we could move forward with what we've got in the meantime? I think what we've got here is better than what we had before and the behavior ("automatic" validation) is largely the same, so we could treat making |
I took a deeper look into this topic and believe to have found a cleaner solution in #520. Please take a look if this approach fits your needs. |
Yep should work |
Currently
poetry lock --no-update
fails when you remove any path dependency because it first loads all dependencies from the lock file (which still has a reference to the dependency you just deleted) and it fails at that point because whenDirectoryDependency
is instantiated with a non-existent path it immediately throws an exception.This change allows the process to continue, which is the same behavior VCS dependencies have.
If the
pyproject.toml
has a path dependency that doesn't exist we will want to error forpoetry install
andpoetry lock
(poetry build
with path dependencies is already kinda a no-go).poetry install
already fails gracefully (if you lock the project then delete the path dependency and try to install; i.e. when no locking happens before we start installing), I opened python-poetry/poetry#6844 to makepoetry lock
fail gracefully.