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

pip list #53

Closed
wants to merge 6 commits into from
Closed

pip list #53

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 10, 2017

No description provided.

@ghost
Copy link
Author

ghost commented Aug 10, 2017

  • fix the Travis test (only append --disable-pip-version-check to the pip list invocation when using a pip version that supports it; recognize and skip the warning in parse_legacy)
  • handle only --verbose, --raw, --interactive and --auto in our parse_args function; forward anything else as-is to pip list
  • add a test to check that pip-review --nonsense fails because pip does not recognize the argument (pip-review should be totally fine with it)

@ghost
Copy link
Author

ghost commented Aug 10, 2017

@jgonggrijp

I know you claimed that:

What you say is true. However, the PR already contains code for taking advantage of --format=json as well as for backwards compatibility. So ditching pip<9 would not speed up the PR; in fact, it would destroy work. Other things need to be addressed in order for the PR to complete.

However, as you can see, the regular expression does not work.

@ghost
Copy link
Author

ghost commented Aug 10, 2017

add a test to check that pip-review --nonsense fails because pip does not recognize the argument (pip-review should be totally fine with it)

Also, I'm not messing with cram.

@jgonggrijp jgonggrijp self-assigned this Aug 10, 2017
@jgonggrijp
Copy link
Owner

jgonggrijp commented Aug 10, 2017

Thanks for taking up on this, @xoviat. I sympathize with your reason to not include the test, so it will not stop me from accepting your PR (instead, I will try to add the test myself). I will give this a proper review today starting at 21:00 UTC+1+DST (my time zone), so about 11 hours from now. At first glance, it looks very promising.

Edit for future reference: this PR resumes where #41 became suspended.

Copy link
Owner

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

There were some minor issues with your code, but I already fixed them on feature/pip-list. You were right that the parse_legacy didn't work, I fixed that as well.

I will add the missing tests (for argument forwarding) and then merge and release this. Thanks for helping pip-review to reach version 1.0!

return parser.parse_args()


parsed, unknown = parser.parse_known_args() #this is an 'internal' method
Copy link
Owner

Choose a reason for hiding this comment

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

No, this is not an internal method, even if you quote it. It is a public, well-documented, official part of the standard library which was included precisely for our purpose. I'm glad you found it.

unknown is a rather confusing name for a variable. I did some renaming and removed your comment in db0f0fc.

# which returns 'parsed', the same as what parse_args() would return
# and 'unknown', the remainder of that
# the difference to parse_args() is that it does not exit when it finds redundant arguments
unknown = [arg for arg in unknown if arg.startswith(("-", "--"))]
Copy link
Owner

Choose a reason for hiding this comment

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

This was a bit overly restrictive. pip has arguments that take values, such as --timeout. The above line removes those values, causing pip list to bail out. I fixed this in 16e8ab4.

version = package["version"]
yield name, parse_version(version), version, False


Copy link
Owner

Choose a reason for hiding this comment

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

Still a joy to see all those lines disappear.

if user:
command.append('--user')
def get_outdated_packages(unknown):
command = ['pip', 'list', '--outdated'] + unknown
Copy link
Owner

Choose a reason for hiding this comment

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

This was still missing a pip_cmd(). I fixed that in 00e58ab.

if parse_version(pip.__version__) > parse_version('9.0'):
command.append('--disable-pip-version-check')
Copy link
Owner

Choose a reason for hiding this comment

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

This actually applies from pip>=6 onwards. I fixed that in 706840c.

if parse_version(pip.__version__) > parse_version('9.0'):
command.append('--disable-pip-version-check')
command.append('--format=json')
output = check_output(" ".join(command)).decode('utf-8')
Copy link
Author

Choose a reason for hiding this comment

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

@jgonggrijp This must have slipped in from @bil-elmoussaoui. We do not want to do this. The command should be passed as a list.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree it should be a list, but I vaguely recall that check_output requires a string argument. It hasn't caused troubles yet, so it's not stopping me from rolling out the 1.0 release, but go ahead and submit a pull request if you can't stand it.

Your fix should branch off from feature/pip-list but you should request to merge into develop, because I locally already merged the branch. Thanks in advance if you do, no problem if you don't. I'm fine with it either way.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, it might actually be older than the commits from @bil-elmoussaoui.

@ghost ghost closed this Aug 11, 2017
@ghost ghost deleted the pip-list branch August 11, 2017 00:43
@jgonggrijp
Copy link
Owner

If you hadn't closed this, it would now have been displayed as merged.

@ghost ghost restored the pip-list branch August 11, 2017 01:35
@ghost ghost deleted the pip-list branch August 11, 2017 01:35
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants