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(linter): add unconvert linter and address related issues #8685

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

hezhizhen
Copy link
Contributor

Problem

Solution

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2023

CLA assistant check
All committers have signed the CLA.

mangalaman93
mangalaman93 previously approved these changes Feb 20, 2023
@mangalaman93
Copy link
Member

@hezhizhen Thanks for PR, LGTM. Will merge it when the test passes.

@mangalaman93 mangalaman93 self-assigned this Feb 20, 2023
@hezhizhen
Copy link
Contributor Author

labeler is failed. @mangalaman93

@hezhizhen
Copy link
Contributor Author

@mangalaman93 Can you re-run failed tests?

@skrdgraph
Copy link
Contributor

@hezhizhen we have an open issue for the code-coverage failure here. This should ideally not be a blocker for your PR, as the test part of the pipeline ran fine. The issue has more details around why it is failing (it's a perceived failure).

Regarding the labeler, I believe it's a similar issue. @MichelDiz can you cross check here?

From what I can say, the PR is in a good shape.

@skrdgraph
Copy link
Contributor

@hezhizhen we have made a fix to our pipelines to address the above issue I had stated. The fixes primarily went in 2 PRs #8717 & #8716. Could you pull the latest main into your branch? This should give you a GREEN tick on all the tests.

Another request, I raised a PR to address this issue - #8705. I don't intend to merge mine. Would it be possible for you to port missing changes from mine into yours? TIA!

@hezhizhen
Copy link
Contributor Author

@hezhizhen we have made a fix to our pipelines to address the above issue I had stated. The fixes primarily went in 2 PRs #8717 & #8716. Could you pull the latest main into your branch? This should give you a GREEN tick on all the tests.

Another request, I raised a PR to address this issue - #8705. I don't intend to merge mine. Would it be possible for you to port missing changes from mine into yours? TIA!

Okay, I'll rebase main and add the missing changes from your PR. Thanks.

@hezhizhen
Copy link
Contributor Author

@skrdgraph I rebased main and checked every change you made in #8705 . I suppose the missing change is nolint:unused in common.go, and I commented there why this is not necessary. Correct me if I'm wrong.

Copy link
Contributor

@skrdgraph skrdgraph left a comment

Choose a reason for hiding this comment

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

@hezhizhen thanks for taking time to do this - appreciate your help. I have validated this on my machine as well.

Sudhishs-MacBook-Pro-Work:tmp sudhishkr$ git clone https://github.com/hezhizhen/dgraph.git
Cloning into 'dgraph'...
remote: Enumerating objects: 70636, done.
remote: Counting objects: 100% (209/209), done.
remote: Compressing objects: 100% (181/181), done.
remote: Total 70636 (delta 56), reused 151 (delta 24), pack-reused 70427
Receiving objects: 100% (70636/70636), 364.04 MiB | 17.84 MiB/s, done.
Resolving deltas: 100% (48207/48207), done.
Sudhishs-MacBook-Pro-Work:tmp sudhishkr$ cd dgraph/
Sudhishs-MacBook-Pro-Work:dgraph sudhishkr$ golangci-lint run --max-same-issues=100
Sudhishs-MacBook-Pro-Work:dgraph sudhishkr$ echo $?
0
Sudhishs-MacBook-Pro-Work:dgraph sudhishkr$ 

PS: if you are interested in contributing further - we do maintain a small todo list OR you can l
Screen Shot 2023-03-03 at 06 28 11
ook through reported issues on GH as well.

@hezhizhen
Copy link
Contributor Author

@hezhizhen thanks for taking time to do this - appreciate your help. I have validated this on my machine as well.

Sudhishs-MacBook-Pro-Work:tmp sudhishkr$ git clone https://github.com/hezhizhen/dgraph.git
Cloning into 'dgraph'...
remote: Enumerating objects: 70636, done.
remote: Counting objects: 100% (209/209), done.
remote: Compressing objects: 100% (181/181), done.
remote: Total 70636 (delta 56), reused 151 (delta 24), pack-reused 70427
Receiving objects: 100% (70636/70636), 364.04 MiB | 17.84 MiB/s, done.
Resolving deltas: 100% (48207/48207), done.
Sudhishs-MacBook-Pro-Work:tmp sudhishkr$ cd dgraph/
Sudhishs-MacBook-Pro-Work:dgraph sudhishkr$ golangci-lint run --max-same-issues=100
Sudhishs-MacBook-Pro-Work:dgraph sudhishkr$ echo $?
0
Sudhishs-MacBook-Pro-Work:dgraph sudhishkr$ 

PS: if you are interested in contributing further - we do maintain a small todo list OR you can l Screen Shot 2023-03-03 at 06 28 11 ook through reported issues on GH as well.

Oh, I didn't notice it. Thanks for reminding me of that.

I'll take a look at those items and try to solve some if I can.

@skrdgraph skrdgraph merged commit 3ae7559 into hypermodeinc:main Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants