Skip to content

chore(linter): add unconvert linter and address related issues#8685

Merged
skrdgraph merged 1 commit intodgraph-io:mainfrom
hezhizhen:unconvert
Mar 3, 2023
Merged

chore(linter): add unconvert linter and address related issues#8685
skrdgraph merged 1 commit intodgraph-io:mainfrom
hezhizhen:unconvert

Conversation

@hezhizhen
Copy link
Copy Markdown
Contributor

Problem

Solution

@CLAassistant
Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

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

labeler is failed. @mangalaman93

@hezhizhen
Copy link
Copy Markdown
Contributor Author

@mangalaman93 Can you re-run failed tests?

@skrdgraph
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 dgraph-io: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