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

Improved error message in ParseException #129

Merged
merged 6 commits into from Jul 18, 2018

Conversation

Projects
None yet
4 participants
@venthur
Copy link
Contributor

venthur commented Apr 6, 2018

Hello,

This PR will help to fix pypa/pip#5147 and pypa/pip#5170. It changes the string being passed in the InvalidRequirement to something like Parse error: Expected stringEnd (at char 5), (line:1, col:6) which can be easily used by pip to produce a nicer error message on parsing errors.

@venthur venthur referenced this pull request Apr 6, 2018

Closed

Fix/5147 #5170

def test_parseexception_error_msg(self):
with pytest.raises(InvalidRequirement) as e:
Requirement("toto 42")
assert "Invalid Requiremnt" not in e

This comment has been minimized.

@di

di Apr 6, 2018

Member

Have you tried running this test without the changes you've made? I'm pretty sure it will still pass, the assertion will never be reached because the line before it raises the InvalidRequirement exception.

I also don't think you can assert whether a string is in an exception like that. It'd probably be better to assert that it's equivalent to an expected string, rather than asserting that it doesn't contain some string.

This comment has been minimized.

@venthur

venthur Jun 8, 2018

Contributor

Updated the test to check for a substring being in the exception now.

raise InvalidRequirement(
"Invalid requirement, parse error at \"{0!r}\"".format(
requirement_string[e.loc:e.loc + 8]))
raise InvalidRequirement("Parse error: {0!r}".format(e))

This comment has been minimized.

@di

di Apr 6, 2018

Member

This changes the exception from:

Invalid requirement, parse error at "'42'"

to:

Parse error: Expected stringEnd (at char 5), (line:1, col:6)

It's not really clear to me how this improves the situation in pypa/pip#5170? Is the idea that pip will now parse this new message and display something more meaningful to the user? I'm not sure that the new message is actually more user-friendly than the old one.

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Jun 7, 2018

My 2 cents on this would be to change it to:

index f87c57c..e8008a6 100644
--- a/packaging/requirements.py
+++ b/packaging/requirements.py
@@ -92,16 +92,16 @@ class Requirement(object):
         try:
             req = REQUIREMENT.parseString(requirement_string)
         except ParseException as e:
-            raise InvalidRequirement(
-                "Invalid requirement, parse error at \"{0!r}\"".format(
-                    requirement_string[e.loc:e.loc + 8]))
+            raise InvalidRequirement("Parse error at \"{0!r}\": {1}".format(
+                requirement_string[e.loc:e.loc + 8], e.msg
+            ))

         self.name = req.name
         if req.url:
             parsed_url = urlparse.urlparse(req.url)
             if not (parsed_url.scheme and parsed_url.netloc) or (
                     not parsed_url.scheme and not parsed_url.netloc):
-                raise InvalidRequirement("Invalid URL given")
+                raise InvalidRequirement("Invalid URL: {0}".format(req.url))
             self.url = req.url
         else:
             self.url = None
  • e.msg is "Expected stringEnd" which might help users debug what happened
  • printing the URL of the requirement could be helpful? maybe.
@venthur

This comment has been minimized.

Copy link
Contributor

venthur commented Jun 8, 2018

Updated my PR to @pradyunsg 's suggestion. And added a test for the new string on an invalid-URL exception.

Requirement("name @ gopher:/foo/com")
assert "Invalid URL: " in e

This comment has been minimized.

@pradyunsg

pradyunsg Jun 8, 2018

Member

This should be dedented by one level. Otherwise, this line isn't executed.

This comment has been minimized.

@pradyunsg

pradyunsg Jun 8, 2018

Member

I'd check "Invalid URL" in str(e) and 'gopher:/foo/com' in str(e)

def test_parseexception_error_msg(self):
with pytest.raises(InvalidRequirement) as e:
Requirement("toto 42")
assert "Expected stringEnd" in e

This comment has been minimized.

@pradyunsg

pradyunsg Jun 8, 2018

Member

Dedent this line as well.

@xavfernandez xavfernandez merged commit 7e8b97a into pypa:master Jul 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xavfernandez

This comment has been minimized.

Copy link
Contributor

xavfernandez commented Jul 18, 2018

@venthur thanks for the PR :)

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