-
Notifications
You must be signed in to change notification settings - Fork 140
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 setuptools integration. #218
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
==========================================
+ Coverage 74.28% 77.83% +3.54%
==========================================
Files 7 8 +1
Lines 350 406 +56
==========================================
+ Hits 260 316 +56
Misses 90 90
Continue to review full report at Codecov.
|
It's a great feature to add :) |
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.
This looks really good. There is one point I would like to discuss anyway.
My understanding is that the setup command, as another interface to Safety, should be adapt itself to such context. So, instead of allowing users to pass any argument as they would to to Safety CLI, just makes it sound like having two ways of doing the same thing. What I am trying to say is that users could then just call the CLI and that's it.
So, my proposal here is to change the arguments interface, and how the command gathers options from setup
call itself, to run the check.
I will give you an example:
- Remove all options except for those changing results format and the key: bare, json, full-report and key
- Bring minimum set of options to
setup
call like: ignore, output, cache, db and files - Make sure we are running a check against
install_requires
dependencies by default.
In a way or another, this already looks great! Looking forward your changes.
|
||
``` | ||
[safety] | ||
full-report yes |
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.
Just curious, don't we need an =
here?
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.
Yes ... yes you do 😞
@@ -147,6 +147,22 @@ and displays a status on GitHub. | |||
|
|||
![Safety CI](https://github.com/pyupio/safety/raw/master/safety_ci.png) | |||
|
|||
## Using Safety from setup.py | |||
|
|||
Safety includes a [distutils extension] that runs the check command. |
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.
Maybe we could explain briefly how the users should configure their setup.py
file, including adding safety to setup_requires
.
from distutils import cmd | ||
import os | ||
|
||
from safety import cli |
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.
Shall we use safety.safety
instead of safety.cli
?
My understanding is that cli
module serves as an interface for the console solely. Setup command would be another interface IMHO.
Using
Does that sound good? |
That would be great, @dave-shawley Looking forward those changes! |
Hi! |
This PR adds a setuptools command for running
safety check
. I'm not sure if this is something that you are interested in or not, but it makes configuration via setup.cfg possible.