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 misspell, nestif golang linter #2240

Merged
merged 44 commits into from
Dec 21, 2023
Merged

Enable misspell, nestif golang linter #2240

merged 44 commits into from
Dec 21, 2023

Conversation

ankitjain235
Copy link
Contributor

@ankitjain235 ankitjain235 commented Aug 3, 2023 •
edited
Loading

Change Overview

This PR enables misspell and nestif golang linters.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

@infraq infraq added this to In Progress in Kanister Aug 3, 2023
@ankitjain235 ankitjain235 marked this pull request as ready for review August 3, 2023 15:28
mellon-collie
mellon-collie suggested changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@mellon-collie mellon-collie 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.

Please rebase the commits to show logical progression, i.e., one commit could be to just enable go linting for misspell and the other commit could be for actually making the linting errors.

Kanister automation moved this from In Progress to Review Required Aug 7, 2023
@mellon-collie
Copy link
Contributor

Should we also raise separate PRs for enabling each of these linters and fixing them? For instance, one PR could be just for enabling misspell and fixing all the misspell lint issues.

@ankitjain235
Copy link
Contributor Author

@mellon-collie I kept them this way so that we can track which changes were made for which enabled linter respectfully. It would be easy to track things for someone who is making changes in the same code path later.
When the PR is merged, then also the git blame would show this as part of linter enable PRs only and won't make users worry that this commit could be causing change in code behaviour. WDYT?

@mellon-collie
Copy link
Contributor

@mellon-collie I kept them this way so that we can track which changes were made for which enabled linter respectfully. It would be easy to track things for someone who is making changes in the same code path later.
When the PR is merged, then also the git blame would show this as part of linter enable PRs only and won't make users worry that this commit could be causing change in code behaviour. WDYT?

Sure, that makes sense. Thanks for clarifying!

PrasadG193
PrasadG193 reviewed Dec 4, 2023
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
.golangci.yml
@@ -37,6 +40,8 @@ issues:
- unparam # Tests might have unused function parameters.
- lll
- dupl
- misspell
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine to enable misspell in test file if doesn't require much changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does require changes. Enabling these linters for tests would need a lot of extra work. I'll create a separate issue for the same?

.golangci.yml Show resolved Hide resolved
pkg/function/create_rds_snapshot.go Show resolved Hide resolved
Kanister automation moved this from Review Required to Reviewer approved Dec 6, 2023
Base automatically changed from enable-linters-3 to master December 21, 2023 04:17
@ankitjain235 ankitjain235 mentioned this pull request Dec 21, 2023
Closed
@ankitjain235 ankitjain235 linked an issue Dec 21, 2023 that may be closed by this pull request
Add more linters to golangci.yaml #2173
Closed
@mergify mergify bot merged commit 0caa0df into master Dec 21, 2023
14 checks passed
Kanister automation moved this from Reviewer approved to Done Dec 21, 2023
@mergify mergify bot deleted the enable-linters-4 branch December 21, 2023 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mellon-collie mellon-collie mellon-collie requested changes

@pavannd1 pavannd1 pavannd1 approved these changes

@PrasadG193 PrasadG193 PrasadG193 approved these changes

@viveksinghggits viveksinghggits Awaiting requested review from viveksinghggits

Assignees
No one assigned
Labels
kueue
Projects
Kanister
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more linters to golangci.yaml
4 participants
@ankitjain235 @mellon-collie @pavannd1 @PrasadG193