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 show --json functionality #5954

Closed
wants to merge 27 commits into from
Closed

Add show --json functionality #5954

wants to merge 27 commits into from

Conversation

MarckK
Copy link

@MarckK MarckK commented Oct 28, 2018

Fixes #5261

@MarckK MarckK force-pushed the json branch 4 times, most recently from 60c602a to 18da610 Compare October 28, 2018 14:49
@MarckK MarckK force-pushed the json branch 6 times, most recently from 6e2e04d to a9dff5e Compare October 29, 2018 08:00
@MarckK MarckK changed the title [WIP]Add show --json functionality Add show --json functionality Oct 29, 2018
@MarckK MarckK force-pushed the json branch 2 times, most recently from a07ba62 to ab329db Compare October 30, 2018 11:25
@brcrista
Copy link
Contributor

@MarckK FYI, I just deleted the "Pip CI (Docs)" pipeline, so that's why I cancelled your build there

@MarckK
Copy link
Author

MarckK commented Nov 22, 2018

Hi @brcrista, Pip CI(macOS) is failing an integration test with python 2.7. Not sure how to resolve. Advice appreciated :)

@pfmoore
Copy link
Member

pfmoore commented Nov 22, 2018

@MarckK The MacOS error is here: https://dev.azure.com/pypa/pip/_build/results?buildId=2521&view=logs (I think...)

@brcrista It's quite hard to get to the actual error details from the "Details" link in the Github CI checks. Is this something that can be improved? Either by documentation or (better!) by configuring things so that the test output is easier to get to?

@MarckK
Copy link
Author

MarckK commented Nov 22, 2018

Hi @pfmoore, thanks, yeah, I found it. Just wasn't sure how to fix the failing test, and the following didn't clarify for me: https://dev.azure.com/pypa/pip/_build/results?buildId=2521&view=logs

@pfmoore
Copy link
Member

pfmoore commented Nov 22, 2018

Ah, OK. Yeah, that test failure looks like pip --force-reinstall isn't updating any files. But on MacOS only, and only for Python 2.7. That's weird.

@dstufft
Copy link
Member

dstufft commented Nov 22, 2018 via email

@pfmoore
Copy link
Member

pfmoore commented Nov 22, 2018

I hit re-run on that test. Let's see what happens...

@pfmoore
Copy link
Member

pfmoore commented Nov 22, 2018

First impression - Github hasn't noticed that I did so...

@brcrista can you help? Why isn't hitting the "re-run" button showing up here?

@MarckK
Copy link
Author

MarckK commented Nov 22, 2018

Hi @dstufft, @pfmoore, @brcrista, I'd love to think it was a flaky test, but I've rebased to update today's changes, and (force) pushed, triggering the tests again, and the same test has failed again.
Pip CI(macOS) is failing an integration test with python 2.7: pip --force-reinstall isn't updating any files.

@brcrista
Copy link
Contributor

Hey everyone, I was gone on the Thanksgiving holiday in the US.

@pfmoore there's a known bug where the "re-run" button on the GitHub PR won't trigger builds from forks. You can still re-run on the Azure Pipelines side (you need to have permission to run a build):

image

@brcrista
Copy link
Contributor

@pfmoore about what you said about the GitHub checks page -- could you explain what info you're looking for here? I see the log lines for the failing test under "Annotations." When I click "Raw output" it shows the output from the failing test.

@BrownTruck
Copy link
Contributor

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 master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 22, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 24, 2019
@cjerdonek
Copy link
Member

I think this PR is getting too long and too hard to review because it's not just adding the json format. There is a lot going on. For example, one important thing is making sure the default format is being preserved correctly in the refactor, and I'm not sure there is adequate test coverage of the default format for us to be falling back on to rely on. Basically, a bunch of refactoring is happening at the same time as an addition of new behavior, and it's hard to separate them. (And there are 27 commits.)

My suggestion would be to start by biting off a smaller chunk. Namely, submit a PR that only refactors the existing implementation into (1) getting the info, and (2) formatting it with the existing format. That PR can also start off by making sure there is adequate test coverage of the default case to make sure the refactor is preserving the output. Then, after that is committed, the second PR would only have to format the info as JSON and add tests only for that.

(Also, if written correctly, I'm not sure you'd even need more than a trivial JSON-specific test, since all the JSON formatter does is call json.dumps(). Most of the testing effort, then, should be going into testing the function that generates the data structure you pass into the JSON formatter or default formatter. Those tests would be useful for both the JSON and default formatter, but I'm not seeing tests of that form here.)

Even as a maintainer I strive for PR's with limited scope and frequently break things up into smaller PR's. It makes writing and reviewing PR's easier for everyone, and makes it easier to make forward progress.

@chrahunt chrahunt added the S: awaiting response Waiting for a response/more information label Aug 16, 2019
@BrownTruck
Copy link
Contributor

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 master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 19, 2019
@chrahunt
Copy link
Member

Hello! Thanks so much for your contribution. It has been some time since this has seen activity, so I will close this PR. This is not saying these changes are not useful, just that it is not something we're considering actively worked on right now.

@MarckK or anyone else interested in picking this up, please don't hesitate to open a new PR including these changes. I would pay special attention to @cjerdonek's comment above - if there's some part of the code which needs to be cleaned up, doing that in a separate PR before the functional change can really help move things along smoothly.

Thanks again!

@chrahunt chrahunt closed this Nov 16, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 16, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 16, 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 needs rebase or merge PR has conflicts with current master S: awaiting response Waiting for a response/more information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip show --json