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 code quality issues #1093

Closed
wants to merge 5 commits into from
Closed

Fix code quality issues #1093

wants to merge 5 commits into from

Conversation

withshubh
Copy link

This pull request fixes some of the code quality issues raised by DeepSource on my fork of this repository. I have fixed some issues using DeepSource's Autofix.

Take a quick look at all the issues caught by DeepSource for this repository here.

Summary of fixes

  • Remove unnecessary comprehension
  • Use literal syntax to create data structure
  • Remove redundant None default

You can also have a look at the configuration file I used for DeepSource Analysis.

@casperdcl
Copy link
Sponsor Member

Thanks. A few questions:

  1. what's the advantage of DeepSource over Codacy?
  2. can the configuration be moved from .deepsource.toml to a [tool.deepsource] section under pyproject.toml?

@withshubh
Copy link
Author

Thanks. A few questions:

  1. what's the advantage of DeepSource over Codacy?
  2. can the configuration be moved from .deepsource.toml to a [tool.deepsource] section under pyproject.toml?

Hey @casperdcl 👋

  1. DeepSource requires a unique single file configuration that saves time and keeps the false-positives rate at less than 5%. DeepSource's auto-fix feature generates PR for the commonly occurring issues, unlike Codacy where you have to manually fix the issue. Some more advantages over Codacy are mentioned here.

  2. As of now we can't move the configuration from .deepsource.toml to the [tool.deepsource] section. It needs to be there.

@casperdcl
Copy link
Sponsor Member

casperdcl commented Dec 6, 2020

Hmm afaik a new Python static analysis tool configured in toml outside the standard pyproject.toml is an anti-pattern and definitely enough of a reason to not merge this unless fixed.

@withshubh
Copy link
Author

Hmm afaik a new Python static analysis tool configured in toml outside the standard pyproject.toml is an anti-pattern and definitely enough of a reason to not merge this unless fixed.

Hi @casperdcl 👋

Since DeepSource works as an additional service (just like Codacy, etc.) that you don’t have to run yourself, we have certain restrictions (just like how you need to create workflow YAML files for GitHub Actions or Circle CI, or the codecov.yml which is already present in the repository).
The scope of what you can do is also more than one programming language: for instance, you can use DeepSource to track test coverage, detect problems in Dockerfiles, etc. in addition to detecting issues in Python. This is why we’ve taken a decision to keep the configuration independent. A number of popular open-source projects, including the likes of isort (https://github.com/pycqa/isort) use DeepSource with the configuration file as is.

@casperdcl casperdcl self-assigned this Jan 9, 2021
@casperdcl casperdcl added c1-quick 🕐 Complexity low p4-enhancement-future 🧨 On the back burner labels Jan 9, 2021
@casperdcl casperdcl added this to the Non-breaking milestone Jan 9, 2021
@casperdcl casperdcl mentioned this pull request Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c1-quick 🕐 Complexity low p4-enhancement-future 🧨 On the back burner
Projects
Casper
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants