-
Notifications
You must be signed in to change notification settings - Fork 3k
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 json formatting option to pip show #7967
Conversation
Hi @chrahunt and @cjerdonek Apologies for the ping but I saw your comments on the last PR attempting to implement I would appreciate it if you guys can shed any thoughts on this, and any necessary scope of improvement :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we'd need an actual json printer and its tests :)
I was planning to implement the json printer and the associated tests in a follow up PR, so as to not crowd this one and making sure the default format tests are passing with the refactoring |
That's definitely a good idea but then I'd say you need to:
|
Thanks for that, I have addressed all the points you made in the latest commits. Please have a look 😊 |
Thanks @xavfernandez for the approval. I will tag you to the follow up PR for this, which will add the Hi @pradyunsg , Could I please get this PR merged as well :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I did not check the code more thoroughly before reviewing but it seems that search_packages_info
is already somewhat performing the role of the new get_package_info
(i.e. collecting information before formatting): it might make for sense to modify the key
/title
(capitalize, etc) there ?
If I understand correctly, I would prefer to keep those two separate, as they were before this change ( What do you think? |
Currently, we have We definitely want those 2 steps separated. But currently in this PR, we now have I think |
I can also do it the way you mentioned where Actually I was thinking earlier that we have But I am not sure which one is better, this or the idea above it? |
I have merged |
I have added the JSON printer, and I have also copied over the unit tests to work with json formatting. Please take a look |
Let's be careful not rushing this out. Once released it will be very hard to change. We need to see if we can be compatible (or at least consistent) with Perhaps it's a case where we'd want to document the output before implementing. Several fields are obvious (yet documenting their relation to standards would help clarify), some other less so. |
Hi @sbidoul
Agreed.
How can we be sure of that. Currently I have taken the default output and mapped it to json directly. The keys are the same along with the casing, and the values are either a string (this is for keys The only exception is
I do see that there is no documentation of what each field refers to in the Once this document is agreed upon, I can just change around the keys and values for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some documentation update would also be needed indeed 👍
@xavfernandez I will create a separate companion PR to include the document changes and link it here. Also do we want to update https://github.com/pypa/pip/blob/master/docs/html/reference/pip_show.rst for this ? |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
To move this PR ahead, I have added some suggestions made at #8008 (comment) into the document PR #8008 I am open to reviews of the document PR, and once that is in an acceptable state, I can start the implementation here according to what we have in the docs. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Hi @xavfernandez , @sbidoul Since #8008 , the documentation PR is at a good stage now, pending discussion on the nitty-grittes of fields, I was wondering if I can start the implementation of the atleast the basic fields in |
@deveshks the safest would be to first make sure that most maintainers are on board with this change before implementing it :) |
Thanks for the tip @xavfernandez . I will hold of on making changes here before getting agreement on what changes need to be made in the doc PR. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Fixes and closes #5261
Json printing logic is added, as well as supporting unit tests. Different outputs with json formats are show below for reference.
Help section of
pip show
Output of
pip show pip --format=header
orpip show pip
Output of
pip show pip --format=json
Output of
pip show pip -v --format=json
Output of
pip show pip -f -v --format=json
(When files can't be located)Output of
pip show tox -v -f --format=json
(When files can be located)