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

chore: refactor code quality issues #2450

Merged
merged 6 commits into from
Apr 5, 2021
Merged

chore: refactor code quality issues #2450

merged 6 commits into from
Apr 5, 2021

Conversation

akshgpt7
Copy link
Contributor

Description

Hey 👋, I'm a Developer Outreach at DeepSource and ran DeepSource analysis on my fork of the repo. It found some interesting code quality improvements to consider.

This PR fixes a few of the issues detected for you to assess if it is right for you.
Happy to provide the tweaks separately otherwise :)

Summary of changes

  • Use literal syntax instead of function calls to create data structures.
  • Refactor unnecessary use of comprehension.
  • Remove redundant None default.
  • Remove assert statement from non-test files.
  • Remove unused imports.
  • Add .deepsource.toml file for continuous analysis on bug risks/performance/code-quality issues on new changes.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR. Always appreciated.

Regarding DeepSource: We currently use Codacy for code quality analysis and IIRC we could even find more issues with that, if we'd tighten the settings. Why should we choose to switch to DeepSource rather than sticking with Codacy or switching to other services like e.g. DeepCode.ai? And I mean apart from you getting paid by them? 😜

PS: Tests fail b/c I revoked some bot tokens yesterday that are used in the tests, will update them soonish

examples/echobot.py Outdated Show resolved Hide resolved
telegram/bot.py Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Mar 30, 2021

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

@Bibo-Joshi
Copy link
Member

I just merged #2451 to master, which fixes the failing tests, so please merge from master.

@akshgpt7
Copy link
Contributor Author

Why should we choose to switch to DeepSource rather than sticking with Codacy or switching to other services like e.g. DeepCode.ai?

@Bibo-Joshi There are various added benefits that DeepSource offers:

  • Automatic code formatting to comply with the code style (through Transformers).
  • Option to Autofix detected issues through one click.
  • DeepSource Python analyzer detects 520 issues as of now (new support being continuously added).
  • Less than 5% false positive reports in DeepSource.
  • Option to report false positives directly.

These are of course some of the benefits, and just for you to assess if the tool is right for the project :)
If you have any questions, feel free to reach out or have a look at the docs.

@Bibo-Joshi
Copy link
Member

Thanks for the info! We'll have to discuss it in the dev team, but I'd like to merge your PR first. Would you mind removing the deepsource config file?

@akshgpt7
Copy link
Contributor Author

akshgpt7 commented Apr 2, 2021

No problem, I've removed the config file @Bibo-Joshi.
If you want, I can open an issue for the same, but it's fine if you want to discuss it internally too :)

@Bibo-Joshi Bibo-Joshi merged commit 9d93417 into python-telegram-bot:master Apr 5, 2021
@Bibo-Joshi
Copy link
Member

Thanks for the contribution @akshgpt7 ! We'll discuss switching in the dev team - your example config file is already a great starting point :)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants