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

add the exhaustive linter, replace panics by return values in logging stringers #2729

Merged
merged 5 commits into from
Oct 5, 2020

Conversation

marten-seemann
Copy link
Member

Depends on #2728.

A while back, @lanzafame suggested to replace the panics in the Stringers of logging types by "unknown" return values. Of course, he was right with that suggestion. Logging is not a good enough reason to panic.

Turns out that there's an even better solution for this: the exhaustive linter, which even is part of the golangci-lint suite. This linter allows us to check that switch statement for enum types cover all values defined for that enum.

There are a few cases in our code where we deliberately don't do an exhaustive check, and I've added a nolint:exhaustive statement there. This is fine, at least it forces us to be explicit about this. In fact, activating this linter actually caught a bug in the packet packer (#2728).

@marten-seemann marten-seemann force-pushed the exhaustive-linter branch 2 times, most recently from 959155c to 7406eaa Compare August 20, 2020 08:09
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #2729 into master will increase coverage by 0.01%.
The diff coverage is 73.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2729      +/-   ##
==========================================
+ Coverage   85.92%   85.93%   +0.01%     
==========================================
  Files         133      133              
  Lines       12075    12087      +12     
==========================================
+ Hits        10375    10386      +11     
- Misses       1365     1366       +1     
  Partials      335      335              
Impacted Files Coverage Δ
internal/protocol/encryption_level.go 90.91% <ø> (-9.09%) ⬇️
internal/protocol/packet_number.go 100.00% <ø> (ø)
qlog/types.go 85.12% <0.00%> (ø)
packet_packer.go 83.68% <80.00%> (+0.12%) ⬆️
internal/wire/transport_parameters.go 87.38% <92.50%> (-0.08%) ⬇️
crypto_stream_manager.go 90.00% <100.00%> (+0.34%) ⬆️
internal/ackhandler/received_packet_handler.go 77.65% <100.00%> (+0.54%) ⬆️
internal/ackhandler/sent_packet_handler.go 73.66% <100.00%> (+0.04%) ⬆️
internal/handshake/crypto_setup.go 65.21% <100.00%> (+0.07%) ⬆️
internal/wire/extended_header.go 86.83% <100.00%> (+0.16%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebe051b...8417b67. Read the comment docs.

Copy link
Member

@lucas-clemente lucas-clemente left a comment

Choose a reason for hiding this comment

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

GH is showing tons of lint errors. Do you want to fix those now or later?

@marten-seemann marten-seemann force-pushed the exhaustive-linter branch 4 times, most recently from 861cd0a to dea4a34 Compare August 21, 2020 06:39
@marten-seemann marten-seemann merged commit c75da79 into master Oct 5, 2020
@marten-seemann marten-seemann deleted the exhaustive-linter branch October 5, 2020 06:59
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
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.

2 participants