Skip to content
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

add support for pip install https://<github-url>.git #580

Closed
wants to merge 6 commits into from

Conversation

mihneadb
Copy link

This would add the functionality requested in [1]. I ran the tests and everything works like it did before the commit.

[1] #539

@travisbot
Copy link

This pull request fails (merged 3bf0c5c into c670678).

@mihneadb
Copy link
Author

The tests results are the same as before. Please review the code.

@travisbot
Copy link

This pull request fails (merged cdc5c3c into c670678).

@travisbot
Copy link

This pull request fails (merged e879b02 into c670678).

@travisbot
Copy link

This pull request fails (merged a4aec42 into c670678).

@travisbot
Copy link

This pull request fails (merged 9bdf120 into c670678).

@travisbot
Copy link

This pull request fails (merged 193b758 into c670678).

@carljm
Copy link
Contributor

carljm commented Jun 17, 2012

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.

@carljm carljm closed this Jun 17, 2012
@mihneadb
Copy link
Author

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
Sent from a mobile device.

On 18.06.2012, at 00:01, Carl Meyerreply@reply.github.com wrote:

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.


Reply to this email directly or view it on GitHub:
#580 (comment)

@carljm
Copy link
Contributor

carljm commented Jun 17, 2012

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?

@mihneadb
Copy link
Author

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
Sent from a mobile device.

On 18.06.2012, at 00:14, Carl Meyerreply@reply.github.com 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?


Reply to this email directly or view it on GitHub:
#580 (comment)

@carljm
Copy link
Contributor

carljm commented Jun 17, 2012

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 #egg= fragment or an @rev after the .git. And it should apply to both editable and non-editable URLS (i.e. whether -e/--editable is used or not).

And it needs a test (or perhaps two, one with -e and one without.

@mihneadb
Copy link
Author

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
Sent from a mobile device.

On 18.06.2012, at 00:31, Carl Meyerreply@reply.github.com 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 #egg= fragment or an @rev after the .git. And it should apply to both editable and non-editable URLS (i.e. whether -e/--editable is used or not).

And it needs a test (or perhaps two, one with -e and one without.


Reply to this email directly or view it on GitHub:
#580 (comment)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants