Skip to content

Raise InvalidRequirement/Marker from exc#593

Closed
Jackenmen wants to merge 3 commits intopypa:mainfrom
Jackenmen:raise_from_none
Closed

Raise InvalidRequirement/Marker from exc#593
Jackenmen wants to merge 3 commits intopypa:mainfrom
Jackenmen:raise_from_none

Conversation

@Jackenmen
Copy link
Copy Markdown
Contributor

@Jackenmen Jackenmen commented Sep 14, 2022 •
edited
Loading

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:
    [email protected]/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:
    [email protected]/hukkin/tomli/archive/master.zip; python_version < '3.11'
                                                     ^

@brettcannon
Copy link
Copy Markdown
Member

brettcannon commented Sep 23, 2022

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
Copy Markdown
Member

brettcannon commented Sep 23, 2022

Do you have an opinion, @pradyunsg ?

@pradyunsg
Copy link
Copy Markdown
Member

pradyunsg commented Sep 24, 2022

I don’t have any opinion either way,

@Jackenmen
Copy link
Copy Markdown
Contributor Author

Jackenmen commented Sep 24, 2022

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
Copy Markdown
Member

brettcannon commented Sep 27, 2022

Then please use the from exc form.

@Jackenmen Jackenmen changed the title Raise InvalidRequirement from None Raise InvalidRequirement/Marker from exc Sep 27, 2022
brettcannon
brettcannon requested changes Oct 6, 2022
View reviewed changes
packaging/requirements.py
req = _RequirementTuple(*parse_named_requirement(requirement_string))
except ParseExceptionError as e:
raise InvalidRequirement(str(e))
raise InvalidRequirement(str(e)) from e
Copy link
Copy Markdown
Member

@brettcannon brettcannon Oct 6, 2022

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

@pradyunsg pradyunsg mentioned this pull request Dec 7, 2022
Merged
@Jackenmen Jackenmen deleted the raise_from_none branch January 5, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon requested changes

Assignees

No one assigned

Labels

enhancement packaging.markers packaging.requirements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Jackenmen @brettcannon @pradyunsg