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

Refactor code quality issues #2044

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Refactor code quality issues #2044

merged 1 commit into from
Mar 5, 2021

Conversation

akshgpt7
Copy link
Contributor

@akshgpt7 akshgpt7 commented Mar 5, 2021

Description

Hi 👋, I ran DeepSource analysis on my fork of the repo, and found some interesting code quality and performance issues that can improve the general code quality and performance here.
This PR fixes a few of the issues detected.

Summary of changes

  • Remove unnecessary len() call for comparison check.
  • Refactor unnecessary use of comprehension.
  • Use literals instead of function calls to improve performance.
  • Use sys.exit() calls instead of bare exit().
  • Remove unnecessary import aliases.
  • Add .deepsource.toml file for continuous analysis on bug risks/performance/code-quality issues on new changes.

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #2044 (472cc00) into master (00a1ee0) will not change coverage.
The diff coverage is 100.000%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2044   +/-   ##
=========================================
  Coverage   91.857%   91.857%           
=========================================
  Files           35        35           
  Lines         3242      3242           
  Branches       557       556    -1     
=========================================
  Hits          2978      2978           
  Misses         179       179           
  Partials        85        85           
Impacted Files Coverage Δ
sanic/websocket.py 82.418% <ø> (ø)
sanic/mixins/listeners.py 100.000% <100.000%> (ø)
sanic/mixins/middleware.py 100.000% <100.000%> (ø)
sanic/worker.py 84.800% <100.000%> (ø)

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 00a1ee0...472cc00. Read the comment docs.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

Nice, this looks like a good change. I am not familiar with this tool. Is this something that we could add to tox and then run with CI?


test_patterns = ["tests/**"]

exclude_patterns = ["docker/**"]
Copy link
Member

Choose a reason for hiding this comment

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

What about the docs folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can be excluded too, but I saw the conf.py file in there, so I didn't add the docs folder here.

@ahopkins ahopkins merged commit 0d2d62e into sanic-org:master Mar 5, 2021
@akshgpt7
Copy link
Contributor Author

akshgpt7 commented Mar 5, 2021

@ahopkins

Is this something that we could add to tox and then run with CI?

DeepSource is a static analysis tool (which runs separately) and not exactly for testing, so it can't be integrated directly with tox. However, you can activate analysis for your repository to continuously analyze and detect bug-risks/performance issues/anti-patterns for each new change introduced (just like the other checks run on a PR). It significantly reduces code review time, and you can also use the Autofix feature to fix them with one click.

If you'd like, here's how you can quickly activate DeepSource to continuously analyze your repository:

  • Sign up on DeepSource and activate analysis for your repository.
  • Create .deepsource.toml configuration which you can use to configure your analysis settings (This PR already added that, but you can edit it any time).
  • Activate/Check your analysis here.

If you have any doubts or questions, you can check out the docs, or feel free to reach out :)

@ahopkins
Copy link
Member

ahopkins commented Mar 5, 2021

Thanks, I'll look into that over the weekend. These changes seemed logical enough, but I would not want autofix enabled.

@akshgpt7
Copy link
Contributor Author

akshgpt7 commented Mar 5, 2021

It's not enabled on autopilot. You just have the additional choice to Autofix some detected issues, or manually fix them, or ignore them altogether. Even if you choose to Autofix a particular issue, it opens a separate PR, rather than directly commiting the changes, so nothing goes through undetected :)

@akshgpt7
Copy link
Contributor Author

akshgpt7 commented Mar 8, 2021

@ahopkins, Just making sure if you need any help in activating analysis?
I ask so because you merged the .deepsource.toml file, which will only work if analysis is activated for that repository (through the steps mentioned above).

@akshgpt7
Copy link
Contributor Author

akshgpt7 commented Apr 5, 2021

@ahopkins just let me know if you need any help in activating :)

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.

2 participants