-
Notifications
You must be signed in to change notification settings - Fork 588
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 #48
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: shubhendra <[email protected]>
Signed-off-by: shubhendra <[email protected]>
Signed-off-by: shubhendra <[email protected]>
Signed-off-by: shubhendra <[email protected]>
…atement Signed-off-by: shubhendra <[email protected]>
Signed-off-by: shubhendra <[email protected]>
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Hi Shubhendra, Thank you very much for the PR! DeepSource sound like a very cool project. Unfortunately, we can not integrate '.toml' to the library as we need to maintain sync between the internal and external versions of the repo. If that is fine with you, I am happy to review the PR assuming we do not push '.toml' file. |
Hi @cyrilchim 👋 No worries! I removed the I would really appreciate it if you could drop any suggestion on what we can do better to onboard you(or tf-quant-finance) on DeepSource. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, Shubhendra.
This all looks good. I have only a minor comment on 'callable'.
Could you please also squash your commit?
Thank you!
# Python 2.7 as well as Python 3.x with x > 2 support 'callable'. | ||
# In between, callable was removed hence we need to do a more expansive check | ||
if hasattr(var_or_fn, '__call__'): | ||
if callable(var_or_fn): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a legacy part to deal with Python 2.7. Since we now require Python >= 3.7, we can replace '_is_callable' with the in-built method 'callable'. Please do so or leave it as it is now
Description
Hi 👋 I work at DeepSource, I ran DeepSource analysis on the forked copy of this repo and found some interesting code quality issues in the codebase, opening this PR so you can assess if our platform is right and helpful for you.
Summary of changes
callable()
to check if the object is calllableelse
/elif
whenif
block has abreak
statementobject
from the base class