pipenv icon indicating copy to clipboard operation
pipenv copied to clipboard

Handle venv_resolve_deps exception

Open sanspareilsmyn opened this issue 1 year ago • 6 comments

The issue

This is an enhancement of error message regards to https://github.com/pypa/pipenv/issues/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

  • [x] Associated issue
  • [x] 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 #.

sanspareilsmyn avatar May 28 '24 09:05 sanspareilsmyn

@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 avatar May 29 '24 01:05 matteius

Also, I think your change here may be better/supersede the need for: https://github.com/pypa/pipenv/pull/6163 Is that also your impression?

matteius avatar May 29 '24 01:05 matteius

@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!

sanspareilsmyn avatar May 29 '24 01:05 sanspareilsmyn

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

sanspareilsmyn avatar Jul 28 '24 14:07 sanspareilsmyn

@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.

matteius avatar Jul 28 '24 14:07 matteius

@matteius Conflict resolved! Thanks for the notice:)

sanspareilsmyn avatar Jul 29 '24 04:07 sanspareilsmyn