Call os.path.normpath to normalize paths for comp #1521
Conversation
There was a problem hiding this comment.
I do think a test would be useful. Otherwise, this new behavior could be removed and tests would continue to pass. What you want is a test that demonstrates a path that without this change would fail the expectation in the bug report.
Also, if you would please in the changelog entry reference the module/function where the change is occurring. The audience for the changelog is someone who uses setuptools/pkg_reaources but doesn’t necessarily know the code base, so try to give that audience the context to understand the effect of the change.
uranusjr
force-pushed
the
normalize-path-normpath
branch
from
de97e49
to
84f1aba
Compare
|
I added a (rather trivial) test, and modified the fragment to describe the behavioural change rather than the code modification. |
| @@ -236,3 +236,8 @@ def test_version_resolved_from_egg_info(self, env): | |||
| req = pkg_resources.Requirement.parse('foo>=1.9') | |||
| dist = pkg_resources.WorkingSet([env.paths['lib']]).find(req) | |||
| assert dist.version == version | |||
|
|
|||
| def test_normalize_path(self, tmpdir): | |||
There was a problem hiding this comment.
In 092ef22, I extracted from your commit just the change for the added test, but as you can see in this build, even on Windows, that test doesn't capture the failure. I think instead of using tmpdir here, you should use a contrived path, something where pkg_resources.normalize_path() produces an unexpected result before the patch and an expected result after.
changelog.d/1519.change.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Perform additional path normalization to ensure path values to a directory is always the same, preventing false positives when checking scripts have a consistent prefix to set up on Windows. | |||
There was a problem hiding this comment.
Prefix with
In pkg_resources.normalize_path, perform additional...
Co-Authored-By: uranusjr <uranusjr@gmail.com>
| 'suffix, parts', | ||
| [ | ||
| # Trailing sep should be normalized. | ||
| ('foo', ('foo')), |
There was a problem hiding this comment.
I doubt this does what you intended. It's equivalent to ('foo', 'foo'),.
There was a problem hiding this comment.
Oops, Python tuple is hard :p
uranusjr
force-pushed
the
normalize-path-normpath
branch
2 times, most recently
from
3eeb0b0
to
fe58530
Compare
uranusjr
force-pushed
the
normalize-path-normpath
branch
from
fe58530
to
9032519
Compare
|
Test cases updated. |
|
We're getting a lot of duplicate reports for this, so I think we should go ahead and merge it. I'd like to cut a release soon. |
Closes #1519.
Pull Request Checklist
I’m not sure how the test should be constructred. It seems silly to just test whether the function is identical to calling the three functions, but any fixed strings would be platform-dependant.