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

Add test and detailed error message for unsupported dependency case #184

Conversation

gbmarc1
Copy link
Contributor

@gbmarc1 gbmarc1 commented Mar 3, 2023

As per #183, if one package has multiple possible dependency versions, the dependency walker may fall into a trap where it discards the final "good" version.
The dependency walker requires quite a refactor to support this, and there are not tests to perform such a refactor in a confident manner.

Meanwhile, I propose this PR that at least details a bit better the problem so that users more easily investigate a work around.

@gbmarc1
Copy link
Contributor Author

gbmarc1 commented Mar 3, 2023

pre-commit.ci autofix

@sonarcloud
Copy link

sonarcloud bot commented Mar 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@radoering
Copy link
Member

@dimbleby Thoughts on this? You don't happen to plan to solve this problem anytime soon? 😉

@dimbleby
Copy link
Contributor

dimbleby commented Mar 4, 2023

I have no plans to make fixes in this plugin.

As to this MR, I'm not convinced that it's actually doing anything very useful. Should we add a log saying "please contribute a fix" for every bug that we hit?

But I don't care to argue about it, if you're minded to merge then go for it.

@gbmarc1
Copy link
Contributor Author

gbmarc1 commented Mar 5, 2023

I can remove the contribute part of this PR. However, I think this more detailed message error identifies where the problem is (this plugging). I spent half a day investigating where the issue was.
When I encountered this error, I was looking if poetry's new minor version introduced a regression (1.32->1.4). I looked if I had an actual problem with my dependencies. The simple RuntimeError was too vague.
This PR make at least clear where the problem is, makes this plugin assumes owns its limitation, adds accountability, add cleanliness and a test!

I am opened to any suggestions to make the above objectives better.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

I think it's good to merge:

  • The error message is more detailed than before. (Dependency walk can fail for at least one other reason.)
  • Mentioning poetry-plugin-export seems to make sense because many users are not aware of the difference.
  • Probably, asking for contributions does not hurt since it shows we are aware of the issue. Of course, that might be a bit controverse and I agree that we shouldn't do it for any bug, but I'm fine with it in this case.

@radoering radoering merged commit 716363e into python-poetry:main Mar 6, 2023
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.

3 participants