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

Raise InvalidRequirement/Marker from exc #593

Closed
wants to merge 3 commits into from
Closed

Raise InvalidRequirement/Marker from exc #593

wants to merge 3 commits into from

Conversation

Jackenmen
Copy link

@Jackenmen Jackenmen commented Sep 14, 2022

This PR uses raise from exc syntax to improve the traceback from:

Traceback (most recent call last):
  File "/home/ubuntu/work/packaging/packaging/requirements.py", line 40, in __init__
    req = _RequirementTuple(*parse_named_requirement(requirement_string))
  File "/home/ubuntu/work/packaging/packaging/_parser.py", line 90, in parse_named_requirement
    error_message="Expected semicolon (followed by markers) or end of string",
  File "/home/ubuntu/work/packaging/packaging/_tokenizer.py", line 111, in expect
    raise self.raise_syntax_error(message=error_message)
  File "/home/ubuntu/work/packaging/packaging/_tokenizer.py", line 140, in raise_syntax_error
    self.position,
packaging._tokenizer.ParseExceptionError: Expected semicolon (followed by markers) or end of string
at position 59:
    tomli @ https://github.com/hukkin/tomli/archive/master.zip; python_version < '3.11'
                                                               ^

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ubuntu/work/packaging/packaging/requirements.py", line 42, in __init__
    raise InvalidRequirement(str(e))
packaging.requirements.InvalidRequirement: Expected semicolon (followed by markers) or end of string
at position 59:
    tomli @ https://github.com/hukkin/tomli/archive/master.zip; python_version < '3.11'
                                                               ^

to:

Traceback (most recent call last):
  File "/home/ubuntu/work/packaging/packaging/requirements.py", line 40, in __init__
    req = _RequirementTuple(*parse_named_requirement(requirement_string))
  File "/home/ubuntu/work/packaging/packaging/_parser.py", line 90, in parse_named_requirement
    error_message="Expected semicolon (followed by markers) or end of string",
  File "/home/ubuntu/work/packaging/packaging/_tokenizer.py", line 111, in expect
    raise self.raise_syntax_error(message=error_message)
  File "/home/ubuntu/work/packaging/packaging/_tokenizer.py", line 140, in raise_syntax_error
    self.position,
packaging._tokenizer.ParseExceptionError: Expected semicolon (followed by markers) or end of string
at position 49:
    tomli@github.com/hukkin/tomli/archive/master.zip; python_version < '3.11'
                                                     ^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ubuntu/work/packaging/packaging/requirements.py", line 42, in __init__
    raise InvalidRequirement(str(e)) from e
packaging.requirements.InvalidRequirement: Expected semicolon (followed by markers) or end of string
at position 49:
    tomli@github.com/hukkin/tomli/archive/master.zip; python_version < '3.11'
                                                     ^

@brettcannon
Copy link
Member

I think I would rather see the change be raise InvalidRequirement from e if there was a desire to suppress the duplicate output rather than toss out the parsing exception. At least for us having the full traceback helps debug issues.

@brettcannon
Copy link
Member

Do you have an opinion, @pradyunsg ?

@pradyunsg
Copy link
Member

I don’t have any opinion either way,

@Jackenmen
Copy link
Author

I mostly made this PR because I found this traceback excessively big considering both exceptions convey the same amount of information. Still, I guess it would make more sense to use raise InvalidRequirement(...) from exc as InvalidRequirement is sort of a wrapper of the underlying exception and that's one of the cases this syntax is meant for. Just let me know what you want me to do and I'll update the PR if necessary.

@brettcannon
Copy link
Member

Then please use the from exc form.

@Jackenmen Jackenmen changed the title Raise InvalidRequirement from None Raise InvalidRequirement/Marker from exc Sep 27, 2022
@@ -39,7 +39,7 @@ def __init__(self, requirement_string: str) -> None:
try:
req = _RequirementTuple(*parse_named_requirement(requirement_string))
except ParseExceptionError as e:
raise InvalidRequirement(str(e))
raise InvalidRequirement(str(e)) from e
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if repeating the string from e gets us anything. I think raising the appropriate exception is important for except clauses, but a person looking at the output can look at the traceback to see what ultimately triggered the exception.

Suggested change
raise InvalidRequirement(str(e)) from e
raise InvalidRequirement from e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants