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 setuptools integration. #218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dave-shawley
Copy link

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.

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #218 into master will increase coverage by 3.54%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
safety/command.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b81f90...abd3819. Read the comment docs.

@3clypse
Copy link

3clypse commented Oct 16, 2019

It's a great feature to add :)

Copy link
Contributor

@rafaelpivato rafaelpivato left a 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:

  1. Remove all options except for those changing results format and the key: bare, json, full-report and key
  2. Bring minimum set of options to setup call like: ignore, output, cache, db and files
  3. 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.

setup.py Show resolved Hide resolved

```
[safety]
full-report yes
Copy link
Contributor

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?

Copy link
Author

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

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

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.

@dave-shawley
Copy link
Author

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:

  1. Remove all options except for those changing results format and the key: bare, json, full-report and key
  2. Bring minimum set of options to setup call like: ignore, output, cache, db and files
  3. 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.

Using install_requires is a perfect default... not sure why I didn't think of that. The main reason that I offered all of the options is to make them configurable via the setup.cfg. It does make sense to remove some of them.

  1. Remove support for the proxy parameters
  2. Remove support for the --stdin parameter
  3. Initialize the list of packages with install_requires
  4. Add packages explicitly specified in the files parameter to the list of packages

Does that sound good?

@rafaelpivato
Copy link
Contributor

That would be great, @dave-shawley

Looking forward those changes!

@Lucas-C
Copy link

Lucas-C commented Dec 15, 2021

Hi!
This looks very useful :)
What's the status of this PR?

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

Successfully merging this pull request may close these issues.

None yet

4 participants