Catch problems with [tool.poetry.scripts] entries#7756
Catch problems with [tool.poetry.scripts] entries#7756christokur wants to merge 7 commits intopython-poetry:masterfrom
Conversation
| msg = f"{exc.args} - Failed to parse script entry point '{script}'" | ||
| raise ValueError(msg) from exc |
There was a problem hiding this comment.
| msg = f"{exc.args} - Failed to parse script entry point '{script}'" | |
| raise ValueError(msg) from exc | |
| raise ValueError(f"Failed to parse script entry point '{script}'") from exc |
There was a problem hiding this comment.
Several lint tools recommend against interpolation when instantiating Exceptions.
Do you really want this?
There was a problem hiding this comment.
Several lint tools recommend against interpolation when instantiating Exceptions.
Sorry, I don't follow. Can you give an example?
My reasoning is that you get the original exception via from exc. Thus, you don't need exc.args it in the derived exception.
There was a problem hiding this comment.
Poetry does not display the stack trace so you have to add the exception info to the message so the user can see what the symptom and cause is in one message
There was a problem hiding this comment.
Also the example
resources.py:45: [EM102] Exception must not use an f-string literal, assign to variable first
There was a problem hiding this comment.
Thanks for the example. I read the reasoning to EM102 and I'm not sure I'm convinced (especially since we are not printing the traceback by default). Normally, we are using string literals and f-strings without assigning it to a variable first, so there is no reason to deviate from that here.
Poetry does not display the stack trace so you have to add the exception info to the message so the user can see what the symptom and cause is in one message
You will see the traceback by running poetry with -vvv. When not in verbose mode, we shouldn't print the original error message since it is not very helpful anyway. If you think that f"Failed to parse script entry point '{script}'" is not enough information, then what about f"Failed to parse script entry point '{script}', ':' not found" or similar?
By the way, we have the same script parsing with the same issue in
poetry/src/poetry/console/commands/run.py
Line 73 in 161b19c
There was a problem hiding this comment.
This also catches cases where someone does not place any ":"
Are we overthinking this?
There was a problem hiding this comment.
Good point, my proposal is not accurate enough. However, I still stand by my initial statement.
IMO ('not enough values to unpack (expected 2, got 1)',) - Failed to parse script entry point '...' is not a good error message. Without any context not enough values to unpack does not really help. Failed to parse script entry point '...' should be good enough. If you want more details you have to run poetry install -v. That way you will get the not enough values to unpack with the line where it happened.
|
came across #5885 and went to see if anyone had done this PR before submitting it myself, wondering why this was closed? seems like a no brainer. Re the prior discussion, it seems like it would be simple to wrap the whole block of three string splits with try: except: and just say I'm pretty sure the "don't fstring in messages" is more for log messages, where you save time by not interpolating if eg. the message won't be processed due to the loglevel |
|
I assume the author lost interest. It shouldn't be too difficult to push it across the finish line if you address the review feedback. |
|
Cool, done #8898 |
|
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. |
4 participants
Pull Request Check List
Resolves: #5885 (partially)
Problem encountered:
Output:
too many values to unpack (expected 2)for current project's pyproject.toml i.e. editable install failedAs mentioned in #5885 these failures are cryptic and hard to diagnose. For example, I tried to run poetry via the debugger but then encounter a lot of challenges with
subprocessinvocations using the debugger options etc.I eventually tracked down the error in my pyproject.toml to this pattern
Solution:
At a low level I did not wish to substantially change the way the low level code deal with exceptions but I did want to add some context value to the error message the user will see. My proposed change to the
_add_scriptmethod follows this idea.I created a test to reproduce the problem, verified that the code changes fixes the problem and added a simple fixture for script related problems like the one I encountered and there are many more test cases we can add there