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

Fixed code quality issues #311

Merged
merged 11 commits into from Mar 12, 2021
Merged

Fixed code quality issues #311

merged 11 commits into from Mar 12, 2021

Conversation

withshubh
Copy link
Contributor

Description

Hi 👋
I ran the DeepSource analysis on the forked copy of this repo and found some interesting code quality issues.

Motivation and Context

Some of the issues I have fixed boost minor performance and a few removes the anti-patterns in code.

Types of changes

  • Fix bugs
  • Add new feature
  • Update documentation

Summary of changes

  • Removed length check in favour of truthiness of the object
  • Removed unnecessary use of comprehension
  • Removed methods with unnecessary super delegation
  • Used literal syntax to create data structure
  • Refactored unnecessary else / elif when if block has a continue statement
  • Refactored the comparison involving not
  • Added .deepsource.toml file

Signed-off-by: shubhendra <withshubh@gmail.com>
Signed-off-by: shubhendra <withshubh@gmail.com>
Signed-off-by: shubhendra <withshubh@gmail.com>
Signed-off-by: shubhendra <withshubh@gmail.com>
Signed-off-by: shubhendra <withshubh@gmail.com>
… statement

Signed-off-by: shubhendra <withshubh@gmail.com>
Signed-off-by: shubhendra <withshubh@gmail.com>
@withshubh withshubh changed the title Add .deepsource.toml Fixed code quality issues Mar 6, 2021
@withshubh withshubh marked this pull request as draft March 8, 2021 07:52
@Derek-Wds
Copy link
Contributor

Hi @withshubh , can you double check your modification since it seems the unit test failed. You can run the unit test with pytest qlib/tests/test_all_pipeline.py under upper directory of qlib.

@withshubh withshubh marked this pull request as ready for review March 12, 2021 07:36
@you-n-g you-n-g merged commit 6d5381f into microsoft:main Mar 12, 2021
@withshubh
Copy link
Contributor Author

Hey @Derek-Wds 👋

The fixes in this PR were made using DeepSource Auto-fix feature.

Can you give DeepSource a try on this repo? ✨
The repo already contains the configuration file. All you need to do is Signup here and then activate analysis here.

Activating analysis will help you to continuously analyze the repo for code quality issues and you can fix most of the fix using auto-fix feature.

@withshubh
Copy link
Contributor Author

Hi @you-n-g @Derek-Wds 👋

Did you try out DeepSource? I'd like to get your feedback ✨

@you-n-g
Copy link
Collaborator

you-n-g commented Apr 3, 2021

Hi @withshubh
Thanks for your adivce.
I setup it just now. But I'm not sure if I did it correctly.

image

@withshubh
Copy link
Contributor Author

Hi @you-n-g

Follow these steps:

  • Install DeepSource on your repository here.
  • Activate analysis here.
  • you don't need to create the .deepsource.toml config file since the repo already has one through this PR.

And there you go 💖

@you-n-g
Copy link
Collaborator

you-n-g commented Apr 3, 2021

@withshubh
Thanks so much!
DeepSource is really a powerful tool.
I can't manage the account of microsoft. But I install DeepSource on my personal account.

It has been installed here

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.

None yet

3 participants