Skip to content

Fix linting - #2903

Merged
gmlewis merged 5 commits into
google:masterfrom
WillAbides:lint
Sep 8, 2023
Merged

Fix linting#2903
gmlewis merged 5 commits into
google:masterfrom
WillAbides:lint

Conversation

@WillAbides

@WillAbidesWillAbides commented Aug 26, 2023

Copy link
Copy Markdown
Contributor

I noticed that .golangci.yml had some commented out and disabled linters. This PR fixes that. It also uncomments "example" from the GitHub action.

Running go mod tidy -compat 1.17 fixed the "no go files to analyze" error linting "examples".1

For the linting issues, I updated .golangci-lint and fixed the issues that it found.

I think most of the changes are obvious, but I want to mention a few things:

  • I created two test helpers "assertNilError" and "assertWrite" because that made the error checks issues easier to fix with find/replace. I see that assert functions are generally not used in this repo. I'm happy to inline these, but my preference is to keep them as-is.
  • I changed the signature of stringifyValue to accept a *bytes.Buffer instead of an io.Writer because the linter knows that Buffer.Write always returns nil errors.
  • I removed quite a bit of dead code from authorizations_test.go

Footnotes

  1. examples/go.* changes were originally part of this PR, but merging Remove dependency on "golang.org/x/oauth2" #2895 removed them from the diff.

@codecov

codecovBot commented Aug 27, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2903 (1f47817) into master (66b1147) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2903 +/- ##
==========================================
+ Coverage 98.14% 98.17% +0.03% 
==========================================
Files 143 143 Lines 12606 12609 +3 ==========================================
+ Hits 12372 12379 +7 + Misses 159 156 -3 + Partials 75 74 -1 
Files ChangedCoverage Δ
github/actions_secrets.go100.00% <100.00%> (ø)
github/github.go98.11% <100.00%> (+<0.01%)⬆️
github/repos_releases.go91.13% <100.00%> (+1.68%)⬆️
github/strings.go100.00% <100.00%> (ø)

📢 Have feedback on the report? Share it here.

@WillAbides

Copy link
Copy Markdown
ContributorAuthor

Codecov didn't like that I modified an uncovered line, so I added a test for it.

@gmlewis

Copy link
Copy Markdown
Collaborator

I'm doing a crazy amount of driving today, so will be slow to respond. I try to "approve run" of the checks when I can, but today's going to be pretty challenging, sorry.

@WillAbides

Copy link
Copy Markdown
ContributorAuthor

@gmlewis I appreciate how responsive you are on this repo. I have no expectation that you or anybody else be around on a weekend. I can see the lint results on my fork, so my feedback loop is just fine.

# Conflicts:
#	example/go.mod
#	example/go.sum

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wow, @WillAbides - thank you for this huge cleanup! It is greatly appreciated!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 28, 2023

@gabriel-samfiragabriel-samfira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is awesome

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 8, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @gabriel-samfira !
Merging.

@gmlewis
gmlewis merged commit fd33b81 into google:masterSep 8, 2023
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto 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

@WillAbides@gmlewis@gabriel-samfira