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

Fix some code quality issues #466

Merged
merged 5 commits into from Aug 26, 2020
Merged

Conversation

pnijhara
Copy link
Contributor

[] I have added an entry to docs/changelog.md

Summary of changes

Fix #459

  • Use sys.exit() calls (Bug-Risk)
  • Remove unnecessary generator (Antipattern)
  • Remove length check in favour of truthiness of the object (Performance)

I have also added include *.toml in MANIFEST.in because Test lints were failing.
Find other issues here → https://deepsource.io/gh/pnijhara/pipx/issues/


This PR also adds .deepsource.toml configuration file to run DeepSource analysis on the repo with. Upon enabling DeepSource, the analysis will run on every PR and commit to detect 560+ types of issues in the changes — including bug risks, anti-patterns, security vulnerabilities, etc.

To enable DeepSource analysis after merging this PR, please follow these steps:

  1. Signup on DeepSource with your GitHub account and grant access to this repo.
  2. Activate analysis on this repo here.

You can also look at the docs for more details. Do let me know if I can be of any help!

Signed-off-by: Prajjwal Nijhara <prajjwalnijhara@gmail.com>
src/pipx/commands/common.py Outdated Show resolved Hide resolved
MANIFEST.in Outdated
@@ -1,5 +1,6 @@
graft src
include *.md pipx_demo.gif logo.png LICENSE
include *.toml
Copy link
Member

Choose a reason for hiding this comment

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

It’s better spell out the file name explicitly since this may cause other TOML files to be bundled accidentally. But do we need to include the analyser configuration in sdist in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will make it .deepsource.toml`. But I have added this because the Test/lint were failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is only to prevent test/lint from failing, you can always specifically exclude the file.

Signed-off-by: Prajjwal Nijhara <prajjwalnijhara@gmail.com>
@pnijhara pnijhara requested a review from uranusjr August 18, 2020 17:34
MANIFEST.in Outdated
@@ -1,5 +1,6 @@
graft src
include *.md pipx_demo.gif logo.png LICENSE
include .deepsource.toml
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't needed in sdist, so this should be exclude.

Signed-off-by: Prajjwal Nijhara <prajjwalnijhara@gmail.com>
Copy link
Contributor

@itsayellow itsayellow left a comment

Choose a reason for hiding this comment

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

Thanks!

@itsayellow
Copy link
Contributor

@uranusjr , @cs01, how do you feel about using DeepSource? This PR also adds .deepsource.toml which only makes sense if we subscribe to DeepSource.

I like the code fixes here, but am personally neutral about DeepSource.

@gaborbernat gaborbernat merged commit 979911a into pypa:master Aug 26, 2020
@cs01
Copy link
Member

cs01 commented Aug 30, 2020

While I am grateful for this PR, I am slightly opposed to it. I don't like that it must be run on their CI systems, and cannot be run locally or on GitHub Actions. Landing it actually didn't change anything because we have to go turn it on at the DeepSource webpage.

The fixes added in this PR more or less amount to bikeshedding. I would rather see more flake8 linters like flake8-eradicate and vulture (https://github.com/jendrikseipp/vulture), which do find real problems, and can be run locally and with GitHub Actions.

@pnijhara
Copy link
Contributor Author

pnijhara commented Sep 4, 2020

Hey @cs01, DeepSource integrates with GitHub checks directly and provides a dashboard for every repo. This helps you view the detailed description of an issue and triage the issues. And won’t break CI builds due to a minor style issue. You don’t need to have any other linter configuration or RC files in the repo. That said, for convenience, we’re working on adding this to the CLI which can be used with CI systems (or) IDE plugins which could be used locally. Support for issues detected by flake8-eradicate will be live in 2-3 weeks. Thanks for that.

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.

Potential code quality issues found
5 participants