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: Enable linters exportloopref, nolintlint, whitespace #535

Merged
merged 9 commits into from
Jun 22, 2022

Conversation

orpheuslummis
Copy link
Contributor

@orpheuslummis orpheuslummis commented Jun 18, 2022

Relevant issue(s)

Resolves #536
Part of #487

Description

Adds simple linters: exportloopref, nolintlint, whitespace.

Removes from the list-of-linters-to-consider:

  • megacheck as it is deprecated
  • gci which is not needed because we enforce gofmt

A potentially controversial decision here is enabling the stylistic whitespace. https://github.com/ultraware/whitespace
This is a draft PR, your feedback is welcome.

I was curious about the list of linters Shahzad had built and this PR results from that.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Test suite ran locally and on CI.

Specify the platform(s) on which this was tested:

  • MacOS

@orpheuslummis orpheuslummis added code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. labels Jun 18, 2022
@orpheuslummis orpheuslummis added this to the DefraDB v0.3 milestone Jun 18, 2022
@orpheuslummis orpheuslummis self-assigned this Jun 18, 2022
@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented Jun 18, 2022

@orpheuslummis orpheuslummis requested a review from a team June 18, 2022 22:17
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Appreciate this PR 😃! Thanks for taking the time to also remove the gci and megacheck, made some minor suggestions.

tools/configs/golangci.yaml Show resolved Hide resolved
tools/configs/golangci.yaml Outdated Show resolved Hide resolved
tools/configs/golangci.yaml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 19, 2022

Codecov Report

Merging #535 (83dc4b2) into develop (bb08df0) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #535      +/-   ##
===========================================
- Coverage    54.42%   54.34%   -0.08%     
===========================================
  Files           97       97              
  Lines        13174    13149      -25     
===========================================
- Hits          7170     7146      -24     
+ Misses        5325     5324       -1     
  Partials       679      679              
Impacted Files Coverage Δ
api/http/handler.go 100.00% <ø> (ø)
client/dockey.go 51.16% <ø> (ø)
datastore/badger/v3/datastore.go 33.92% <ø> (ø)
datastore/wrappedStore.go 62.77% <ø> (-0.27%) ⬇️
db/collection_delete.go 60.27% <ø> (-1.41%) ⬇️
db/collection_update.go 42.98% <ø> (-0.13%) ⬇️
db/fetcher/dag.go 56.79% <ø> (-0.53%) ⬇️
db/fetcher/fetcher.go 61.03% <ø> (ø)
db/fetcher/versioned.go 49.21% <ø> (-0.27%) ⬇️
net/dialer.go 3.12% <ø> (ø)
... and 17 more

tools/configs/golangci.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

I'm happy with this, but from memory someone in the team likes more whitespace than I do (including at top of functions maybe) - Only Shahzad is added to this PR atm, make sure the rest of the team is happy too.

@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented Jun 20, 2022

I'm happy with this, but from memory someone in the team likes more whitespace than I do (including at top of functions maybe) - Only Shahzad is added to this PR atm, make sure the rest of the team is happy too.

I did request PR review from the database-team group. I'm assuming this sends email to everyone, and if my expectation is wrong I'd like to know :)

@AndrewSisley
Copy link
Contributor

I did request PR review from the database-team group. I'm assuming this sends email to everyone, and if my expectation is wrong I'd like to know :)

Ah! I only see Shahzads name in the list of reviewers lol

@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented Jun 20, 2022

For the record, Andy and I discussed and we figured out that when we request review from database-team, all members of database-team subsequently receive all notifications on a PR.

@fredcarle
Copy link
Collaborator

For the record, Andy and I discussed and we figured out that when we request review from database-team, all members of database-team subsequently receive all notifications on a PR.

Is that a bad thing?

@fredcarle
Copy link
Collaborator

I like almost everything about this. The rest I don't care. So overall that's a win in my book.

@orpheuslummis
Copy link
Contributor Author

For the record, Andy and I discussed and we figured out that when we request review from database-team, all members of database-team subsequently receive all notifications on a PR.

Is that a bad thing?

IMO is a good thing

@orpheuslummis orpheuslummis marked this pull request as ready for review June 20, 2022 23:28
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing all the requested changes!

@orpheuslummis orpheuslummis merged commit 88e46d6 into develop Jun 22, 2022
@orpheuslummis orpheuslummis deleted the orpheus/chore/linter-investigation branch June 22, 2022 02:02
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. code quality Related to improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable straightforward linters exportloopref, nolintlint, whitespace
4 participants