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.

Sign up for GitHub

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

chore: Refactor code quality issues #23445

Merged
merged 7 commits into from
Mar 27, 2021
Merged

chore: Refactor code quality issues #23445

merged 7 commits into from
Mar 27, 2021

Conversation

powerexploit
Copy link
Contributor

Description

Hey 👋 , I'm member of the Developer Outreach team 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 :)

What problem does this PR solve?

This PR improve some code quality issues like anti-pattern and bug risks.

What is changed and how it works?

  • Remove unnecessary use of slice
  • Fix check for empty string
  • Remove unnecessary wrapping of function call
  • Added .deepsource.toml file for continuous analysis on bug risks/performance/code-quality issues on new changes.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

@powerexploit
Added .deepsource.toml to fix bug risks/performance/code-quality issues
5609738 
Signed-off-by: ankitdobhal <dobhal.ankit@protonmail.com>
@powerexploit
Remove unnecessary use of slice
5c88deb 
Signed-off-by: ankitdobhal <dobhal.ankit@protonmail.com>
@powerexploit
Fix check for empty string
12a019a 
Signed-off-by: ankitdobhal <dobhal.ankit@protonmail.com>
@powerexploit
Remove unnecessary wrapping of function call
6c5369f 
Signed-off-by: ankitdobhal <dobhal.ankit@protonmail.com>
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2021
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Mar 20, 2021
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 20, 2021
@ti-chi-bot
Copy link
Member

Welcome @ankitdobhal!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to pingcap/tidb. 😃

@powerexploit
Copy link
Contributor Author

/cc @AilinKid

@powerexploit powerexploit marked this pull request as ready for review March 20, 2021 05:46
@powerexploit powerexploit requested a review from a team as a code owner March 20, 2021 05:46
@powerexploit powerexploit requested review from wshwsh12 and removed request for a team March 20, 2021 05:46
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2021
@aierui
Copy link
Contributor

aierui commented Mar 20, 2021

mark

@powerexploit
Copy link
Contributor Author

powerexploit commented Mar 23, 2021 •
edited
Loading

/cc @AndreMouche

@ti-chi-bot ti-chi-bot requested review from AndreMouche and removed request for AilinKid March 23, 2021 05:18
xhebox
xhebox reviewed Mar 23, 2021
View reviewed changes
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

.deepsource.toml Outdated
@@ -0,0 +1,11 @@
version = 1
Copy link
Contributor

@xhebox xhebox Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should exclude this file, for now. As we did not integrate deepsource into CI yet, it is possibly better to integrate it into CI after solving all problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xhebox should I remove the .toml file ?

Copy link
Contributor

@xhebox xhebox Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I personally prefer to remove. And I think PR will get merged easier if it is removed. This file may be controversial.

@andylokandy I've shared deepsource to devs. But there is no discussion yet. Maybe you could file an issue like #22979, but for deepsource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xhebox removed .toml file.

@powerexploit
Copy link
Contributor Author

/cc @bb7133

@ti-chi-bot ti-chi-bot requested a review from bb7133 March 24, 2021 06:13
@powerexploit
Removed .deepsource.toml
bac217f 
Signed-off-by: ankitdobhal <dobhal.ankit@protonmail.com>
@xhebox
Copy link
Contributor

xhebox commented Mar 26, 2021

/cc @tangenta

@xhebox
Copy link
Contributor

xhebox commented Mar 26, 2021

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 26, 2021
@tangenta
Copy link
Contributor

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • tangenta
  • xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 27, 2021
@tangenta
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: bac217f

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 27, 2021
@ti-chi-bot ti-chi-bot merged commit 1e3921a into pingcap:master Mar 27, 2021
@powerexploit powerexploit mentioned this pull request Apr 2, 2021
Open
@powerexploit
Copy link
Contributor Author

Hey @xhebox , @tangenta I have opened up issue #23827 regarding to integrate deepsource into CI.
Kindly have a look into issue.
Thanks and regards.

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

@xhebox xhebox xhebox left review comments

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

@AndreMouche AndreMouche Awaiting requested review from AndreMouche

@bb7133 bb7133 Awaiting requested review from bb7133

@tangenta tangenta Awaiting requested review from tangenta

Assignees
No one assigned
Labels
component/expression first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@powerexploit @ti-chi-bot @aierui @xhebox @tangenta @ti-srebot