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: apply go vet improvements #8620

Merged
merged 1 commit into from
Jan 20, 2023
Merged

chore: apply go vet improvements #8620

merged 1 commit into from
Jan 20, 2023

Conversation

mangalaman93
Copy link
Member

No description provided.

@mangalaman93 mangalaman93 mentioned this pull request Jan 19, 2023
Merged
Base automatically changed from aman/imports to main January 20, 2023 07:44
@mangalaman93 mangalaman93 force-pushed the aman/vet branch 2 times, most recently from b7921e2 to caceabb Compare January 20, 2023 07:53
@skrdgraph
Copy link
Contributor

Could you add a description to the PR? There is a PR template that guides certain standards, the description goes into the commit message.

Regarding go vet:

We punted on making these changes because it had turned out to be too much work then. Now that you are making these changes w/ go vet ... it would be best to uncomment this section to see if everything passes correctly. And if it finds any failures you would know right away.

There are other code simplification linters we should start looking into enabling as well.

@skrdgraph
Copy link
Contributor

re-ran the ci-dgraph-tests, because I am seeing some ACL test failures.

@mangalaman93
Copy link
Member Author

I have enabled them in this change. Let me see if we can fix all of them. I had only fixed some of them to begin with. For the PR description, I think there is nothing more that I could describe. The changes are not significant either yet.

sanjayk-github-dev
sanjayk-github-dev previously approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@sanjayk-github-dev sanjayk-github-dev left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic changes.

Were the files under contrib/teamcity deleted because teamcity is not being used?

@mangalaman93
Copy link
Member Author

yes, I am hoping that is fine. If not, I can undo that change.

skrdgraph
skrdgraph approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@skrdgraph skrdgraph left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good 👍 .. thanks for doing this.

@coveralls
Copy link

Coverage Status

Coverage: 66.951% (+0.2%) from 66.77% when pulling c2ecaea on aman/vet into cbec230 on main.

sanjayk-github-dev
sanjayk-github-dev approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@sanjayk-github-dev sanjayk-github-dev left a comment

Choose a reason for hiding this comment

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

Assume that the teamcity related files are not needed any more.

@mangalaman93 mangalaman93 merged commit caed2cb into main Jan 20, 2023
@mangalaman93 mangalaman93 deleted the aman/vet branch January 20, 2023 21:47
@skrdgraph skrdgraph mentioned this pull request Jan 24, 2023
Merged
all-seeing-code pushed a commit that referenced this pull request Jan 24, 2023
@skrdgraph
fix(linter): address gosimple linter reports & errors (#8628)
2212e3f 
GoLinter failures: 
- related to #8620 &
https://github.com/dgraph-io/dgraph/actions/runs/3971511349/jobs/6815792372
- Addressing this partially
#8572 (comment)
- fix `gosimple` & `unused` linter reports
- comment the rest to address later
all-seeing-code pushed a commit that referenced this pull request Feb 8, 2023
@skrdgraph @all-seeing-code
fix(linter): address gosimple linter reports & errors (#8628)
7d59d22 
GoLinter failures: 
- related to #8620 &
https://github.com/dgraph-io/dgraph/actions/runs/3971511349/jobs/6815792372
- Addressing this partially
#8572 (comment)
- fix `gosimple` & `unused` linter reports
- comment the rest to address later
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrdgraph skrdgraph skrdgraph approved these changes

@sanjayk-github-dev sanjayk-github-dev sanjayk-github-dev approved these changes

@akon-dey akon-dey Awaiting requested review from akon-dey

@MichelDiz MichelDiz Awaiting requested review from MichelDiz

@matthewmcneely matthewmcneely Awaiting requested review from matthewmcneely

@darkn3rd darkn3rd Awaiting requested review from darkn3rd

@meghalims meghalims Awaiting requested review from meghalims

@billprovince billprovince Awaiting requested review from billprovince

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mangalaman93 @skrdgraph @coveralls @sanjayk-github-dev