Fix #1664: Include path in error message when version is missing

[cjerdonek]

Fixes #1664.

[cjerdonek]

@pypa/setuptools-developers On this PR, would you be okay with me making also the argument passed to ValueError be a single string instead of the two arguments ValueError(msg, self)?

setuptools/pkg_resources/__init__.py

Lines 2666 to 2667 in 702f2d9

	tmpl = "Missing 'Version:' header and/or %s file"
	raise ValueError(tmpl % self.PKG_INFO, self)

This will make the exception easier to read for users because it won't introduce additional escaping of slashes in the path due to calling repr() on the string.

[pganssle]

Thanks for this PR; I haven't had a chance to review it carefully though, but it looks very thorough!

On this PR, would you be okay with me making also the argument passed to ValueError be a single string instead of the two arguments ValueError(msg, self)?

Hm, I find it a little weird when errors set an args tuple instead of a string as the message anyway, so I don't have a big problem with changing it conceptually, the question is whether anyone is using the fact that it currently raises a tuple?

Might be a little overboard, but maybe we could do this for "best of both worlds"?

class MissingVersionError(ValueError):
    def __repr__(self):
        return "%s(%s, %s)" % (self.__class__.name + self.args)

Haven't looked at this too much, maybe it should be something like InvalidMetadataError instead, but you get my drift - keep the result of .args the same and make the REPR look however you want.

[cjerdonek]

Okay, thanks, @pganssle. I'll think about it what I can do here to preserve that aspect, along the lines you suggest.

I was already planning to propose a new ValueError exception type as a follow-on PR (because it would e.g. let pip handle errors more gracefully without having to risk catching all ValueError objects). It requires a bit more consideration which is why I wanted to propose it separately, but I can start thinking about that here.

[cjerdonek]

Okay, I gave this a bit of thought. To keep the scope of this PR limited, I'll keep everything the same here like the type of the args passed to ValueError and only add the path to the string portion (as in my original PR). In a follow-on PR we can discuss and settle on creating a new exception type (since that will involve some bikeshedding, etc).

In the meantime, I adjusted the tests I created to also check that the args passed to ValueError have the expected type (str for the first one, and a particular Distribution sub-type for the second one). Those tests will come in handy for the follow-on PR.

How does that sound?

[pganssle]

@cjerdonek I'm fine with either thing, tightly scoped PR or bigger one. I think your proposed course of action is prudent.

It seems like there's a test failure on Windows. Hard for me to tell what's going on there, but I think it's related to the fact that windows uses backslashes for path separators, and if you're testing against the repr, you're getting something double-escaped.

[cjerdonek]

It seems like there's a test failure on Windows.

Oops, it was working before, and I didn't check after my last force-push. (I had reversed two positional arguments metadata_path and expected_text when calling check_version_missing().)

It is ready now.

[pganssle]

This seems like a lot of internal changes for a fairly minor convenience feature, but I do think a lot of what you have done is just a bit of refactoring of existing methods in a way that is independent of the necessary change (boy scout rule), so I think that's mostly fine.

Other than that, I have left a few specific comments in-line. Sorry for the delay in this review.

[cjerdonek]

Thanks for all of your comments, @pganssle. I've updated the PR with all of your suggested changes.

[cjerdonek]

Hi @pganssle, just pinging you on this. I answered the question that you asked before, and also included another test case in response to your question.

[pganssle]

OK, this looks good. I would like to re-word the changelog a bit, but I'll do that as part of the merge. Thanks @cjerdonek and sorry again for the delay in approving/merging this.

[cjerdonek]

You might want to reference #6177 and/or #6283 on pip’s tracker as well.

[pganssle]

@cjerdonek Thanks, updated the commit message. Not sure why the pypy job is failing. Seems unrelated? Hopefully restarting the job enough times will work.

[cjerdonek]

Yes, seems unrelated. Are other PR’s failing?

[cjerdonek]

Could the PR need to be rebased? (not sure if master is auto-merged prior to running tests)

[pganssle]

@cjerdonek I rebased it when I was messing around with the commit history, though I think Travis does auto-merge the base branch if possible.

I have occasionally seen intermittent multi-threading issues like this in pypy in the past. I seem to remember that they were resolved upstream, though, so maybe this is something different.

[pganssle]

Thanks @cjerdonek, sorry for the delays and that last hiccup!

[cjerdonek]

Thank you, @pganssle! 🎉This will help users a lot by letting them quickly locate and diagnose problematic installs.

[pganssle]

@cjerdonek This is included in the 40.9.0 release - hot off the presses!