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

Allow check to ignore packages #2408

Merged
merged 7 commits into from Jun 25, 2018

Conversation

Projects
None yet
3 participants
@erinxocon
Contributor

erinxocon commented Jun 25, 2018

and append it to the call to safety

@erinxocon

This comment has been minimized.

Contributor

erinxocon commented Jun 25, 2018

this is a fix for #2407, we were collecting the arguments passed but not actually passing them to safety

@erinxocon

This comment has been minimized.

Contributor

erinxocon commented Jun 25, 2018

needs test

@@ -2409,9 +2409,14 @@ def do_check(three=None, python=False, system=False, unused=False, args=None):
python = which('python')
else:
python = system_which('python')
try:
ignore = ' '.join(args)

This comment has been minimized.

@jarshwah

jarshwah Jun 25, 2018

Using ignore here is confusing, because there could be other args. Consider using safety_args instead?

This comment has been minimized.

@erinxocon
@erinxocon

This comment has been minimized.

Contributor

erinxocon commented Jun 25, 2018

@jarshwah I renamed the parameter option to pipenv check --safety_ignore [package_name]. Is that ok?

@erinxocon erinxocon changed the title from check now see's if --ignore is in the args list to Allow check to ignore packages Jun 25, 2018

@jarshwah

This comment has been minimized.

jarshwah commented Jun 25, 2018

@erinxocon it won't allow callers to ignore multiple issues. Can you make safety_ignore args a list, and then explode to multiple -i args internally?

Something like this perhaps?

'-i '.join(vid for vid in safety_ignore_list)

See: https://github.com/pyupio/safety#--ignore--i

@erinxocon

This comment has been minimized.

Contributor

erinxocon commented Jun 25, 2018

ahh ok, I wasn't familiar with the api for safety. Hold please

@jarshwah

This comment has been minimized.

jarshwah commented Jun 25, 2018

No problem!

For what it's worth, I don't think (m)any of the other CLI options to safety would be useful from pipenv so it might be worth creating a new option directly in the check cli function. That would probably make it easier to implement too with http://click.pocoo.org/5/options/#multiple-options

@erinxocon

This comment has been minimized.

Contributor

erinxocon commented Jun 25, 2018

Ok @jarshwah, I've updated the syntax to pipenv check --safety-ignore [package1] [package2] [package3].... I don't know if we need to make a new click option for this. I'll pin @kennethreitz regarding an api change like that.

EDIT: I eat my words, adding an option was exactly what I ended up doing haha, thanks @jarshwah

@jarshwah

This comment has been minimized.

jarshwah commented Jun 25, 2018

Turn around time on this one was super fast, thanks so much!

erinxocon and others added some commits Jun 25, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 25, 2018

(I removed a print statement)

Does it make sense to change this to --ignore if it's going directly on the safety API? Sorry for the back and forth, I just didn't think through the implementation before I suggested the change :(

Slight tweak to api, add test, add news
Signed-off-by: Dan Ryan <dan@danryan.co>
@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 25, 2018

Swapped it over to --ignore and added some tests + news entry, going to merge this before release

@techalchemy techalchemy merged commit b46feb1 into master Jun 25, 2018

3 checks passed

VSTS: pipenv-Python Package-CI 20180625.3 succeeded
Details
buildkite/pipenv Build #526 passed (6 minutes, 33 seconds)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@techalchemy techalchemy deleted the #2407 branch Jun 25, 2018

@erinxocon

This comment has been minimized.

Contributor

erinxocon commented Jun 25, 2018

thanks @techalchemy, I should have created the news. Sorry for leaving print in there, I was debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment