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

Jump to bottom

fix: code quality issues #406

Merged
merged 4 commits into from Feb 25, 2021
Merged

fix: code quality issues #406

merged 4 commits into from Feb 25, 2021

Conversation

withshubh
Copy link
Contributor

Description

This PR fixes a few issues that were affecting the code quality.

Summary of changes

  • Refactor unnecessary else / elif when if block has a return statement
  • Remove unused imports
  • Use literal syntax to create data structure

rmk135
rmk135 requested changes Feb 25, 2021
View reviewed changes
Copy link
Member

@rmk135 rmk135 left a comment

Choose a reason for hiding this comment

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

@withshubh , thanks, that's fantastic! Two things before merging:

  1. Take a look at my comment on removing fastapi.Request import.
  2. Target the PR to develop branch.

tests/unit/samples/wiringfastapi/web.py Outdated
@@ -1,7 +1,6 @@
import sys

from fastapi import FastAPI, Depends
from fastapi import Request # See: https://github.com/ets-labs/python-dependency-injector/issues/398
Copy link
Member

Choose a reason for hiding this comment

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

Could you, please, leave this import? There was a bug when importing Request crashed wiring. Follow the link for the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted the change ✨

@withshubh withshubh changed the base branch from master to develop February 25, 2021 14:43
@withshubh
Copy link
Contributor Author

@rmk135 Thanks for review! 💖

I have made the changes!

@rmk135 rmk135 merged commit 43eb15e into ets-labs:develop Feb 25, 2021
@withshubh
Copy link
Contributor Author

withshubh commented Feb 25, 2021 •
edited

@rmk135
I would like to share that I ran DeepSource analyzer on my fork of this repository to detect these issues. Have a look at the issues caught in this repository by DeepSource here.

DeepSource is a code review automation tool that detects code quality issues and helps you to automatically fix some of them. You can use DeepSource to track test coverage, Detect problems in Dockerfiles, etc. in addition to detecting issues in code.

Can I submit an issue as a feature request for DeepSource integration or PR(consisting of a configuration file only)?
You can merge the configuration file to continuously analyze the repo and fix code quality issues.

@rmk135
Copy link
Member

rmk135 commented Feb 25, 2021

Thanks for the contribution @withshubh .

I've updated changelog 95b0356 and added your name to the list of contributors 3cf14c1.

DeepSource tool looks good. The only concern I have now is that it seems too noisy. I'm good to give it a try in a recommendation manner. You're welcome to make a configuration file PR.

@withshubh withshubh mentioned this pull request Feb 25, 2021
Merged
@withshubh
Copy link
Contributor Author

Thanks for the contribution @withshubh .

I've updated changelog 95b0356 and added your name to the list of contributors 3cf14c1.

Thank You so much 😍

DeepSource tool looks good. The only concern I have now is that it seems too noisy. I'm good to give it a try in a recommendation manner. You're welcome to make a configuration file PR.

Here it is #407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmk135 rmk135 rmk135 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@withshubh @rmk135