-
Notifications
You must be signed in to change notification settings - Fork 3.1k
add support for pip install https://<github-url>.git #580
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
Conversation
The tests results are the same as before. Please review the code. |
The test results are not the same as before - tests are passing on develop branch in Travis CI, your pull request breaks them. Also, there are several unrelated changes in this pull request. Make unrelated changes in separate branches, and submit them in separate pull requests. |
I didn't know how to make separate pull requests. Will use branches. Regarding the tests, it really is the same on my machine. I'll look into it. Thanks Mihnea Dobrescu-Balaur On 18.06.2012, at 00:01, Carl [email protected] wrote:
|
Hi Mihnea - thanks for the contribution, and thanks for following up to try to get it in mergeable shape. The tests should all pass on the mainline "develop" branch - if they don't on your machine, that indicates a problem with your test setup that you may want to resolve before submitting pull requests. Do you have git, subversion, bazaar, and mercurial all installed? |
Hello Carl, I followed the steps on your website but I developed on a Mac. I'll try tomorrow on my Linux setup. I'll figure it out somehow. After i fix that I willmsplit things into branches. One more thing, regarding coding style: do you prefer inline code or a separate method (see the commit)? Thanks Mihnea Dobrescu-Balaur On 18.06.2012, at 00:14, Carl [email protected] wrote:
|
A separate method is fine, but I don't think this fix is at the right location. It tries to apply itself to all arguments, including those that are not URLs at all, and most importantly it would miss URLs in a requirements file; it's quite important that the acceptable URL formats in a requirements file are the same as those given directly at the command line. So the fix here needs to be deeper into the code, at the point where a requirement (whether commandline or from a reqs file) is identified as a URL requirement. Also, see my note on #539 - the parsing needs to be more sophisticated than a simple "endswith", such that it could properly handle a URL with an And it needs a test (or perhaps two, one with |
I did not intend to cover those, just what the issue's text requested (pip install ). But OK, I will improve it. Mihnea Dobrescu-Balaur On 18.06.2012, at 00:31, Carl [email protected] wrote:
|
This would add the functionality requested in [1]. I ran the tests and everything works like it did before the commit.
[1] #539