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

Enable more revive linter rules #5506

Open
yurishkuro opened this issue Jun 2, 2024 · 3 comments
Open

Enable more revive linter rules #5506

yurishkuro opened this issue Jun 2, 2024 · 3 comments
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Jun 2, 2024 •
edited
Loading

In #5505 we introduced a revive linter, but had to disable many of its rules because they were breaking on some places in the code.

jaeger/.golangci.yml

Line 154 in b38d2f9

rules:

We can re-enable those rules incrementally by fixing the corresponding breaks one rule at a time (make lint should succeed) and removing the disabling entry from the config.

Strong advice: DO NOT OPEN A PR without discussing the impact of the specific linter on this issue first. We don't want very possible linter - some of them are stupid, others require a lot of code changes with minimal benefits.
@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Jun 2, 2024
@hellspawn679 hellspawn679 mentioned this issue Jun 2, 2024
Closed
4 tasks
@FlamingSaint
Copy link
Member

Is it okay to remove multiple rules in the same PR ?

@yurishkuro
Copy link
Member Author

Only if the changes are really small. I would recommend not mixing different changes / rules in one PR.

This was referenced Jun 3, 2024
Merged
Merged
Merged
Merged
@hellspawn679 hellspawn679 mentioned this issue Jun 3, 2024
Closed
4 tasks
@FlamingSaint FlamingSaint mentioned this issue Jun 3, 2024
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Jun 3, 2024
@FlamingSaint
Enable Lint Rules: bare-return & empty-lines (#5512)
bd33dc2 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled bare-return in revive linter. 
- Removed naked returns.
- Enabled empty-lines in revive linter.

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
yurishkuro pushed a commit that referenced this issue Jun 3, 2024
@FlamingSaint
Enable Lint Rule: dot-imports (#5513)
3f82224 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled dot-imports in revive linter. 
- Removed dot imports in a few and added qualifiers for the rest.

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
@FlamingSaint FlamingSaint mentioned this issue Jun 3, 2024
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Jun 3, 2024
@FlamingSaint
Enable Lint Rule: unused-receiver (#5521)
f56409c 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled unused-receiver in revive linter
- Removed receiver names which are not used in functions 

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
yurishkuro pushed a commit that referenced this issue Jun 3, 2024
@FlamingSaint
Enable Lint Rule: modifies-value-receiver (#5517)
c74aec1 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled modifies-value-receiver in revive linter. 
- Removed instances where pass-by-values where re-assigned and returned.

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
This was referenced Jun 3, 2024
Merged
Merged
Merged
yurishkuro added a commit that referenced this issue Jun 4, 2024
@FlamingSaint @yurishkuro
Enable Lint Rules: confusing-results & receiver-naming (#5524)
2438ec0 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled confusing-results & receiver-naming in revive linter
- Used named outputs when datatype of outputs are same
- Made changes to maintain the same receiver name 

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
yurishkuro added a commit that referenced this issue Jun 4, 2024
@FlamingSaint @yurishkuro
Enable Lint Rule: exported (#5525)
7dea2db 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled exported in revive linter
- Converted struct `Config` to `ConfigurationV2`
- Converted struct `DependencyStoreParams` to `Params`
- Converted struct `SamplingStoreParams` to `Params`

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
yurishkuro added a commit that referenced this issue Jun 4, 2024
@FlamingSaint @yurishkuro
Enable Lint Rules: early-return & indent-error-flow (#5526)
6b95711 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled  early-return & indent-error-flow in revive linter
- Simplified if-else blocks

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
This was referenced Jun 4, 2024
Merged
Merged
yurishkuro pushed a commit that referenced this issue Jun 5, 2024
@FlamingSaint
Enable Lint Rules: struct-tag & unexported-return (#5533)
14197fe 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled struct-tag & unexported-return in revive linter
- Only use tags on exported fields
- Converted exported function to have an exported return type

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`
  -

---------

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
@varshith257 varshith257 mentioned this issue Jun 5, 2024
Closed
4 tasks
yurishkuro pushed a commit that referenced this issue Jun 7, 2024
@FlamingSaint
Enable Lint Rule: unused-parameter (#5534)
3d4b6b6 
## Which problem is this PR solving?
-  Partial Fix for #5506

## Description of the changes
- Enabled unused-parameter in revive linter
- Removed or replaced unused parameters with ( _ )
- Added comments for unclear parameters 

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`
  -

---------

Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
@ZXYxc ZXYxc mentioned this issue Jul 30, 2024
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Aug 16, 2024
@ZXYxc
Enable Lint Rule: redefines-builtin-id (#5791)
8921d0b 
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->

## Which problem is this PR solving?
- Partial Fix for #5506

## Description of the changes
- Enabled redefines-builtin-id in revive linter. Replaced function name
which redefinition of the built-in function

## How was this change tested?
- make lint make test

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: ZXYxc <reisaruxxii@gmail.com>
JaredTan95 pushed a commit to JaredTan95/jaeger that referenced this issue Aug 28, 2024
@ZXYxc @JaredTan95
Enable Lint Rule: redefines-builtin-id (jaegertracing#5791)
6e3c890 
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->

## Which problem is this PR solving?
- Partial Fix for jaegertracing#5506

## Description of the changes
- Enabled redefines-builtin-id in revive linter. Replaced function name
which redefinition of the built-in function

## How was this change tested?
- make lint make test

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: ZXYxc <reisaruxxii@gmail.com>
Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@inosmeet inosmeet mentioned this issue Oct 18, 2024
Merged
4 tasks
@inosmeet
Copy link
Contributor

jaeger/.golangci.yml

Lines 199 to 201 in fe700fe

# definitely a good one, needs cleanup first
- name: argument-limit
disabled: true

Enabling this doesn't break anything, so does that mean it's working fine?

yurishkuro pushed a commit that referenced this issue Oct 23, 2024
@inosmeet
Enable Lint Rule: import-shadowing (#6102)
f9474f9 
## Which problem is this PR solving?
- Partial fix for #5506  
## Description of the changes
- Enabled `import-shadowing` in revive linter.
- Changed ambiguous attribute names.

## How was this change tested?
- `make lint` `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`

---------

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
@Rohanraj123 Rohanraj123 mentioned this issue Nov 23, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@yurishkuro @FlamingSaint @inosmeet