Skip to content

Fix issue #131 - Please add status command#664

Merged
carljm merged 7 commits intopypa:developfrom
rafaelcaricio:develop
Sep 8, 2012
Merged

Fix issue #131 - Please add status command#664
carljm merged 7 commits intopypa:developfrom
rafaelcaricio:develop

Conversation

@rafaelcaricio
Copy link
Contributor

Mostly code made by @vbabiy with some modifications and tests. Fix issue #131.

@camilonova
Copy link

+1 Looks good

@hltbra
Copy link
Contributor

hltbra commented Sep 8, 2012

Just as a note: the initial patch was made by @kelseyhightower, not @vbabiy. @vbabiy just migrated pip's issues from Bitbucket to Github (that's why his nick appears as author).

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this assert wrong? Shouldn't it be:

assert 'Location: %s' % dist.location == lines[3]

@carljm carljm merged commit 63459a2 into pypa:develop Sep 8, 2012
@carljm
Copy link
Contributor

carljm commented Sep 8, 2012

I'm sure there could be bikeshedding of the command name and whether it should be part of a larger pip info command, but I've gone ahead and merged this as-is (added some docs and a few tweaks to the tests). I don't consider us committed to this exact command name and CLI UI until we release it, so if anyone wants to argue for changes we can still make them.

Thanks @kelseyhightower for the original work and @rafaelcaricio for adding tests!

@carljm carljm mentioned this pull request Sep 8, 2012
@rafaelcaricio
Copy link
Contributor Author

Thanks for accept my pull request @carljm. And thanks to @hltbra for the comments about my code.
I agree with you that maybe this command could be a part of an pip info command and a better CLI UI could be created. I just left the output as is in the original version.

@jezdez
Copy link
Member

jezdez commented Sep 11, 2012

@carljm I've made my argument for a list command in #235 and think this pull request shouldn't have been merged.

@carljm
Copy link
Contributor

carljm commented Sep 11, 2012

@jezdez Nothing in your proposal on #235 for a list command covers or refers to the functionality provided here. I'm not at all convinced those should be the same command - the hypothetical list is about getting summary information from a longer list of packages, where most of the proposed flags are different ways of defining the exact list you want. In contrast, status is about getting detailed information (including the full list of installed files) from just one or two specifically-named packages; you wouldn't want that much detail from a long list of packages anyway, so it doesn't make sense to make it part of a list command. It's like the difference between aptitude search and aptitude show.

In any case, I think we need to put higher priority on "whoever builds the bikeshed gets to paint it", or else useful features just stall forever, get out of date, need to be rebased, etc. If there's a contribution that works, has tests, and is useful (like this one), I think we should err on the side of merging it sooner rather than later. If someone wants to bikeshed the name etc, they can make their own subsequent contribution of actual code to do that before it is released.

@carljm
Copy link
Contributor

carljm commented Sep 11, 2012

@jezdez I should clarify that I totally agree with your proposal for list on #235; I just see this pull request as a different feature from that, that should be its own command.

@kelseyhightower
Copy link
Contributor

@hltbra Wow, nice to see this finally get merged!

@jezdez
Copy link
Member

jezdez commented Sep 12, 2012

@carljm Oh, true indeed, that was completely different. I wish there wouldn't be another generic subcommand though --"status" and "list" both are pretty ambiguous terms. I would have preferred to have a discussion about those commands again since when @kelseyhightower wrote that end of 2010 we certainly had different plans for pip than now.

Regarding the bikeshedding, I strongly disagree that a working patch and tests is enough to add a feature. This is plain and simple the road to feature creep and an unneeded burden to our already strained time resources. Note, I also very much dislike seeing contributions going stale but I often have regretted adding a feature without having it discussed or thought through before.

If it's not obvious, my opinion isn't meant to be a blow at @kelseyhightower's or @rafaelcaricio's contribution, on the contrary I just want to make sure your contribution is a good as possible and then a bit more.

@carljm
Copy link
Contributor

carljm commented Sep 12, 2012

@jezdez Ok, I think we can easily compromise re approach to merge - in the future I'll highlight you first and ask your opinion. It's particularly tempting to merge quickly when at a sprint and working with the contributor in person :-)

Re the specifics here, I do wonder whether show might be a clearer (and slightly less generic) name for this than status - what do you think?

@jezdez
Copy link
Member

jezdez commented Sep 12, 2012

@carljm Understood, and totally agreed about sprints :)

show sounds much better to me, too. +1

@carljm
Copy link
Contributor

carljm commented Sep 12, 2012

@jezdez changed it to show in 8c9a241

@carljm carljm mentioned this pull request Sep 12, 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.

6 participants