Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Don't append #egg=foo to URLs in missing hashes output (fixes #76) #77

Merged
merged 1 commit into from
Apr 16, 2015
Merged

Don't append #egg=foo to URLs in missing hashes output (fixes #76) #77

merged 1 commit into from
Apr 16, 2015

Conversation

edmorley
Copy link
Contributor

The existing conditional was incorrect (it appended #egg=foo even if it was already present, due to filename_from_url() stripping away the fragment) and should have been replaced by a self._url().endswith() or similar.

However peep already enforces that URLs have the package name specified ("Unable to determine package name from URL ...; add #egg=") so by the time we reach this point, self._url() will always have the package name specified & we'll never need to append it.

@willkg
Copy link
Collaborator

willkg commented Mar 12, 2015

This could use tests. I think it's fine to land after that.

For reference, I added the #egg= part in peep a while back when I added github url support. This was before a non-trivial rewrite. At the time I added this, it was needed.

@edmorley
Copy link
Contributor Author

Does a test make sense for this change?
We're just testing we removed a feature?

@willkg
Copy link
Collaborator

willkg commented Mar 12, 2015

The test should make sure it prints out the right stuff for the various url possibilities. So, I think it's worth a test.

@edmorley
Copy link
Contributor Author

I agree that a bunch of tests for this code is a good idea, but no tests for it was pre-existing, and unfortunately I don't think I'm going to have the time to figure out the peep tests and what the various URL permutations are (I only started using peep today, and I'm still not that familiar with pip install options), so someone else will need to drive this :-)

@willkg willkg self-assigned this Mar 12, 2015
@willkg
Copy link
Collaborator

willkg commented Mar 12, 2015

I'll try to find time to look into this this week. I can at least test the various urls I wrote the original code to support.

@edmorley
Copy link
Contributor Author

Thank you :-)

@willkg
Copy link
Collaborator

willkg commented Mar 12, 2015

Thank you for reporting it and writing up a PR!

@willkg
Copy link
Collaborator

willkg commented Mar 18, 2015

I started writing up tests, but haven't finished, yet. I need to get some other things done, then I'll circle back to this.

If someone wants to grab this from me, let me know.

The existing conditional was incorrect (it appended #egg=foo even if it
was already present, due to filename_from_url() stripping away the
fragment) and should have been replaced by a self._url().endswith() or
similar. However peep already enforces that URLs have the package name
specified ("Unable to determine package name from URL ...; add #egg=")
so by the time we reach this point, self._url() will always have the
package name specified & we'll never need to append it.
@erikrose erikrose merged commit 1e7ad98 into erikrose:master Apr 16, 2015
@willkg
Copy link
Collaborator

willkg commented Apr 16, 2015

I have part of the tests stuff working, but the whole "DownloadReq ratchets forward thing" makes for really tricky testing. I don't know if I'll get that working any time soon.

Anyhow, sorry for blocking this so long.

@edmorley
Copy link
Contributor Author

Thank you :-)

@edmorley edmorley deleted the duplicate-package-name branch April 17, 2015 08:43
@erikrose
Copy link
Owner

And you!

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

Successfully merging this pull request may close these issues.

3 participants