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

Allow check to ignore packages #2408

Merged
merged 7 commits into from
Jun 25, 2018
Merged

Allow check to ignore packages #2408

merged 7 commits into from
Jun 25, 2018

Conversation

erinxocon
Copy link
Contributor

and append it to the call to safety

@erinxocon
Copy link
Contributor Author

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

@erinxocon
Copy link
Contributor Author

needs test

pipenv/core.py Outdated
@@ -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)

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erinxocon
Copy link
Contributor Author

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 check now see's if --ignore is in the args list Allow check to ignore packages Jun 25, 2018
@jarshwah
Copy link

@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
Copy link
Contributor Author

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

@jarshwah
Copy link

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
Copy link
Contributor Author

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
Copy link

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

@techalchemy
Copy link
Member

(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 :(

@techalchemy
Copy link
Member

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
@techalchemy techalchemy deleted the #2407 branch June 25, 2018 07:45
@erinxocon
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants