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

Don't fail on skipped file/dir dependencies #8549

Merged
merged 9 commits into from
Nov 19, 2023

Conversation

Pwuts
Copy link
Contributor

@Pwuts Pwuts commented Oct 17, 2023

Pull Request Check List

Resolves: #8548

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

As suggested by @dimbleby: #8548 (comment)

@dimbleby
Copy link
Contributor

(plus corresponding unit test)

was the full suggestion!

@Wladastic
Copy link

I'd update this branch but I have no access currently^^

As there are no poetry tests we implemented, I guess this can be approved, but documentation could be added that dependencies can be skipped.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 4, 2023

As there are no poetry tests we implemented, I guess this can be approved

I don't follow what you're saying: but there are testcases that could and should be written to go with this code change.

test_install_path_dependency_does_not_exist() is probably a good starting point.

@Wladastic
Copy link

Given that tests/integration/test_setup.py could be augmented as well then, given we have no way to know if setup works on individual systems like Ubuntu, Debian, Windows 10, Windows 11, MacOS, etc.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 4, 2023

I still can't follow your point: but poetry's CI pipelines run across a fairly wide range of platforms.

Are you trying to make a case that there is something special about this code that should make it either untestable, or not worth testing? Or that it needs extra testing? What is it that you think is special about this code?

@Pwuts Pwuts marked this pull request as draft November 8, 2023 16:06
@Pwuts
Copy link
Contributor Author

Pwuts commented Nov 8, 2023

I'm not familiar with this codebase so writing tests is a challenge (with and without AI assistance). Standby while I try to figure this out.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 8, 2023

I think your current draft is not the right direction. I'd try to copy test_install_path_dependency_does_not_exist() in test_install.py (nb not test_installer.py), that looks really quite close to being what you want already.

the bit that's maybe confusing about that test - at least in my opinion - is that it non-obviously refers to example projects eg missing_directory_dependency is an actual project at tests/fixtures/missing_directory_dependency.

hopefully not too hard to set up another fixture project that looks like the #8548 example, and write a test showing that it can be installed when an irrelevant extra is missing, but not when a relevant extra is missing

@Pwuts Pwuts marked this pull request as ready for review November 15, 2023 19:34
@Pwuts
Copy link
Contributor Author

Pwuts commented Nov 15, 2023

TODO:

  • Add poetry.lock file

@Pwuts
Copy link
Contributor Author

Pwuts commented Nov 15, 2023

There we go. Thanks for the guidance @dimbleby! :)

@Pwuts
Copy link
Contributor Author

Pwuts commented Nov 18, 2023

Good to go now? :)

@dimbleby
Copy link
Contributor

looks good to me, but you need someone with the power to press the merge button

@Wladastic
Copy link

Please update the branch first, looks good.

@Pwuts
Copy link
Contributor Author

Pwuts commented Nov 19, 2023

looks good to me, but you need someone with the power to press the merge button

Oh I thought that was you, forgive me for the misunderstanding :')

@radoering radoering merged commit 2900107 into python-poetry:master Nov 19, 2023
19 checks passed
@Pwuts
Copy link
Contributor Author

Pwuts commented Nov 20, 2023

Awesome! Can I ask when we may expect a release containing this fix?

@radoering
Copy link
Member

No plans so far. I wouldn't expect one this year.

MrGreenTea pushed a commit to MrGreenTea/poetry that referenced this pull request Dec 18, 2023
Copy link

github-actions bot commented Mar 3, 2024

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 Mar 3, 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 install fails when irrelevant extra is not installable
4 participants