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

Handle errcheck warnings #932

Merged
merged 1 commit into from Jul 6, 2024
Merged

Handle errcheck warnings #932

merged 1 commit into from Jul 6, 2024

Conversation

ghost
Copy link

@ghost ghost commented Jun 18, 2024

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Go Version Update
  • Dependency Update

Description

The package ignored errors from net.Conn Set*Deadline in a few places. Update the package to return these errors to the caller.

Ignore all other errors reported by errcheck. These errors are safe to ignore because

  • The function is making a best effort to cleanup while handling another error.
  • The function call is guaranteed to succeed.
  • The error is ignored in a test.

Related Tickets & Documents

  • Related Issue #
  • Closes #

Added/updated tests?

  • Yes
  • No, and this is why: covered by existing tests
  • I need help with writing tests

Run verifications and test

  • make verify is passing
  • make test is passing

@ghost ghost mentioned this pull request Jun 18, 2024
1 task
Copy link
Member

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

I have a few pointers on this that I think need solving before merge.

client.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
jaitaiwan
jaitaiwan previously approved these changes Jul 1, 2024
@jaitaiwan
Copy link
Member

@canelohill I had to manually resolve a conflict. Can you check and make sure that fits what you think should happen?

@ghost
Copy link
Author

ghost commented Jul 2, 2024

The PR looks good after merges.

@jaitaiwan
Copy link
Member

@canelohill can’t seem to merge it says there’s conflicts

The package ignored errors from net.Conn Set*Deadline in a few places.
Update the package to return these errors to the caller.

Ignore all other errors reported by errcheck. These errors are safe to
ignore because
- The function is making a best effort to cleanup while handling another
  error.
- The function call is guaranteed to succeed.
- The error is ignored in a test.
@ghost
Copy link
Author

ghost commented Jul 5, 2024

The PR should merge clean now.

@jaitaiwan jaitaiwan merged commit 3810b23 into gorilla:main Jul 6, 2024
1 check passed
@jaitaiwan
Copy link
Member

Cheers thanks for that. I was surprised cause usually I get the opportunity to resolve conflicts manually but this time there was no option.

@ghost ghost deleted the prerrcheck branch July 6, 2024 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant