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 "list" pip command #675

Closed
wants to merge 40 commits into from
Closed

Conversation

rafaelcaricio
Copy link
Contributor

This request was originally made by @dgladkov in the issue #235. I just get all the contributions made in the another issue (by @maraujop and @dgladkov) and extended the idea implementing my understanding about a idea for this command made by @jezdez in the same issue. I didn't implemented all the commands because I wanna to hear from you.

I think this command is extremity useful for the all users and I think that command could fit better to pip than my other contribution #664. What I'm trying to say is that maybe a extension in this command can fit better. What do you think guys? I can make more modifications in this pull request if necessary, just tell me what I can do.

Cheers,

@rafaelcaricio
Copy link
Contributor Author

Travis-ci where are you?

@carljm
Copy link
Contributor

carljm commented Sep 13, 2012

@rafaelcaricio This is looking great, thanks for working on it! I probably can't do a full review this week, but I'll try to get to it early next week.

As I mentioned on #664 I think pip list and pip show should be separate commands, because I think "get extended details from one or two packages, defaulting to no packages" is a basically different thing from "get a summary list of installed packages, defaulting to all-installed". But if you have a specific proposal for how the pip show functionality could be embedded into pip list in a reasonable way, I'm certainly open to considering it.

Also, somehow my "rename 'status' command 'show'" commit got rebased into this pull request's history with a different commit hash; not sure how that happened.

@rafaelcaricio
Copy link
Contributor Author

Hm, I did a "git rebase" in order to keep my branch updated with the original develop branch. So why it happend is discussed here.

@rafaelcaricio
Copy link
Contributor Author

Hello,
Any news about it?

req.check_if_exists()
yield req

def find_packeages_latests_versions(self, options):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"packeages" is a mis-spelling, should be "packages"

@carljm
Copy link
Contributor

carljm commented Sep 28, 2012

Hi @rafaelcaricio - sorry I didn't get to review this as quickly as I thought I would. I just looked over it now and it looks good to me, apart from the minor comments I made. At some point the duplication could be factored out between the options to this command and the options to install; have some kind of common set of "options for commands that use PackageFinder" defined somewhere. This connects to discussion that was also happening on #684. But I don't think it's an issue that needs to hold up this getting in.

I'm actually leaving pip/virtualenv development, so I don't want to actually merge-and-run here and leave the other devs to deal with it :-) But it has my +1 if @jezdez or @brosner or @qwcode or @pnasrat feels like merging it.

@pnasrat
Copy link
Contributor

pnasrat commented Oct 1, 2012

If you can ensure it's mergable I'll happily take another look.

@qwcode
Copy link
Contributor

qwcode commented Oct 1, 2012

@pnasrat , @rafaelcaricio I was going to look at this, but since paul is on the case, I'll just focus on other pulls.

@rafaelcaricio
Copy link
Contributor Author

Hi @pnasrat, I guess this code is mergable. If you want, we can just wait the travisci to run all the tests to verify.

No problem @carljm, thank you very much for your code review. :)

Thanks @qwcode for your attention.

@qwcode
Copy link
Contributor

qwcode commented Oct 2, 2012

I'm guessing the rename commit is the issue with mergability (the rename is already in develop). I cloned your repo and was able to rebase list_command against the latest upstream develop to drop that commit. try that and repush

@rafaelcaricio
Copy link
Contributor Author

Hi @qwcode is it what you wanted to me to do?

@qwcode
Copy link
Contributor

qwcode commented Oct 2, 2012

oh, I'm wondering if what I saw last night was the "merge with caution" due to the tests failing, and not the mergability warning. in any case, the rebase looks good, but that one test that's failing... looks like travis randomness, but do you care to force push again with some minor comment change to get a clean run?

@rafaelcaricio
Copy link
Contributor Author

Ok @qwcode, now it is passing! :)

@qwcode
Copy link
Contributor

qwcode commented Dec 11, 2012

@rafaelcaricio so it looks like all the commits are duplicated in here. hmm.
maybe at this pt, it would be easier if I just grab your commits, and handle the things I mentioned and get it merge ready. you ok with that?

@rafaelcaricio
Copy link
Contributor Author

@qwcode I just did something wrong. :(
Ok, I did not had time yesterday to take care of it and I'm still busy with other things.

@qwcode
Copy link
Contributor

qwcode commented Dec 12, 2012

@rafaelcaricio , no worries. looks like you merged with gvalkov's branch, which was a rebase (which results in new commits), so you ended up with the duplication.

I'll open another pull today or tomorrow that grabs your first set of commits, and handles the remaining details. then get a final kumbaya from folks, and merge.

@qwcode
Copy link
Contributor

qwcode commented Dec 14, 2012

probably be saturday when I do this...

@qwcode
Copy link
Contributor

qwcode commented Dec 16, 2012

closing this one to make sure this doesn't get merged with the duplicate commits.
I'm working on the new pull now using the work that @rafaelcaricio has done so far plus fixes and docs.

@qwcode qwcode closed this Dec 16, 2012
@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.