Skip to content

Comments

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

Merged
radoering merged 8 commits intopython-poetry:mainfrom
gbmarc1:chore/improve-error-message-for-unhandled-dependency-case
Mar 6, 2023
Merged

Add test and detailed error message for unsupported dependency case #184
radoering merged 8 commits intopython-poetry:mainfrom
gbmarc1:chore/improve-error-message-for-unhandled-dependency-case

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

@sonarqubecloud
Copy link

sonarqubecloud 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

radoering commented Mar 4, 2023

@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 •
edited
Loading

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.

radoering
radoering approved these changes Mar 6, 2023
View reviewed changes
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
@radoering radoering mentioned this pull request Apr 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gbmarc1 @radoering @dimbleby