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

Handle venv_resolve_deps exception #6166

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

Conversation

sanspareilsmyn
Copy link
Contributor

@sanspareilsmyn sanspareilsmyn commented May 28, 2024

The issue

This is an enhancement of error message regards to #6073.
Inside do_lock method, exceptions in venv_resolve_deps wasn't properly handled by far. This PR handles exception to improve this problem. Since RuntimeError raised inside venv_resolve_deps can't be easily modified, (it's related to so many components) it raises sys.exit(1) when RuntimeError occurs, and additional traceback log is added on general Exception.

The fix

  • Add error handling logic inside venv_resolve_deps function.
  • Enhance error log.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@matteius
Copy link
Member

@sanspareilsmyn looks like a code linting error. I recommend installing pre-commit and then running pre-commit install in the root checkout of the repo. To correct existing files, run: pre-commit run --all-files --verbose --show-diff-on-failure

@matteius
Copy link
Member

Also, I think your change here may be better/supersede the need for: #6163
Is that also your impression?

@sanspareilsmyn
Copy link
Contributor Author

@matteius Thank you for noticing me. It was due to missing backtick in news fragment. I haven't read your draft PR in prior, but I think handling error in a way I suggest may be more general and runtime-safe to catch other potential corner cases!

@oz123 oz123 requested a review from matteius June 13, 2024 09:18
@sanspareilsmyn
Copy link
Contributor Author

@matteius Hi! Do you mind if you review this PR? Thank you:)

@matteius
Copy link
Member

@matteius Hi! Do you mind if you review this PR? Thank you:)

There are now conflicts with the code that changes -- it needs to be rebased from main.

@sanspareilsmyn
Copy link
Contributor Author

@matteius Conflict resolved! Thanks for the notice:)

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.

2 participants