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

Remove lint exceptions and fix remaining issues #1438

Merged
merged 5 commits into from
May 7, 2024

Conversation

silaselisha
Copy link
Contributor

  • I have removed lint exception by commenting it out
  • Fixed all remaining issues regarding golangci-lint exception

@silaselisha silaselisha requested a review from a team as a code owner April 22, 2024 12:31
@silaselisha silaselisha requested review from roger2hk and removed request for a team April 22, 2024 12:31
@silaselisha
Copy link
Contributor Author

@roger2hk in the previous PR created to solve issues #1438 I realized it was failing as a result of a file of type *os.File getting closed twice and that's in file loglist_refresher_test.go on function createTempFile.

@AlCutter
Copy link
Member

Thanks @silaselisha, I think we might want to "downgrade" some of these exits/panics - I think it's fine to do that in tests, but particularly in longer-lived server processes or client libraries it's likely better to just log these.

Copy link
Contributor

@roger2hk roger2hk left a comment

Choose a reason for hiding this comment

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

Error strings should not be capitalized (unless beginning with an exported name, a proper noun or an acronym) and should not end with punctuation. This is because error strings usually appear within other context before being printed to the user.

https://google.github.io/styleguide/go/decisions#error-strings

@silaselisha
Copy link
Contributor Author

@AlCutter will it be fine for the recommended "downgrading" changes being part of this PR?

@silaselisha
Copy link
Contributor Author

@roger2hk I'll look into it A.S.P. and also should I make changes to pieces of code that I didn't introduce but broke this rule https://google.github.io/styleguide/go/decisions#error-strings?

@roger2hk
Copy link
Contributor

@roger2hk I'll look into it A.S.P. and also should I make changes to pieces of code that I didn't introduce but broke this rule https://google.github.io/styleguide/go/decisions#error-strings?

You should scope it to the lint changes. If you find some other error strings and would like to make changes, please open a separate pull request to address them.

@AlCutter
Copy link
Member

@AlCutter will it be fine for the recommended "downgrading" changes being part of this PR?

@silaselisha yes please

@silaselisha silaselisha requested a review from roger2hk April 30, 2024 16:39
@AlCutter
Copy link
Member

AlCutter commented May 7, 2024

/gcbrun

@AlCutter
Copy link
Member

AlCutter commented May 7, 2024

Hi Silas, many thanks for your PR & updates - I'm just going to make a couple of small tweaks and then I'll merge it for you.

@silaselisha
Copy link
Contributor Author

@AlCutter Thank you, and I'm open for more contribution.

@AlCutter
Copy link
Member

AlCutter commented May 7, 2024

/gcbrun

@AlCutter
Copy link
Member

AlCutter commented May 7, 2024

/gcbrun

trillian/integration/ct_hammer/main.go Outdated Show resolved Hide resolved
trillian/integration/ct_integration.go Outdated Show resolved Hide resolved
@AlCutter
Copy link
Member

AlCutter commented May 7, 2024

/gcbrun

@AlCutter AlCutter merged commit a8de77a into google:master May 7, 2024
6 checks passed
@silaselisha silaselisha deleted the exceptions branch May 9, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants