Impr/more linters

[joeturki]

This enables all linters relevant to Pion, updates the golangci-lint version, and removes deprecated linters.

Updated repos:

• pion/webrtc Upgrade linters webrtc#2985
• pion/turn
• pion/transport
• pion/template
• pion/stun
• pion/srtp
• pion/sdp
• pion/sctp
• pion/rtp Improve the RTP package rtp#288
• pion/rtcp
• pion/randutil
• pion/peerconnection_explainer
• pion/opus
• pion/mdns
• pion/logging Upgrade golangci, More linter logging#116
• pion/interceptor
• pion/ice
• pion/example-webrtc-applications
• pion/dtls

[Sean-Der]

Fantastic!

If you clone the individual Pion repos and run that version of golangci-lint do you get new errors?

Once all the repos are fixed we can bump here!

[joeturki]

Yes, A lot actually :) but I'm fixing everything, I might update the settings a bit if I see issues (like duplicates).

[Sean-Der]

That makes sense! Want to open a PR to fix settings? Happy to approve right away

I don’t want to break things for existing code though. If we enable more linters no one can land code until this is all done

[joeturki]

After giving it some thought, I think this is the best approach:

1. I'll manually modify the .golangci.yml file for each repo independently to test it with the merge actions.
2. Once I’ve updated every repo, we can simply merge this draft PR, and it will apply the updated configuration to all repos, overwriting the current .golangci.yml files. (Does that sound right, Im not sure if Pion's CI/CD works like this?)

The main issue right now is that some linters require minor refactoring. For example:

examples\ortc\main.go:186:28: Magic number: 5, in <argument> detected (mnd) 
	ticker := time.NewTicker(5 * time.Second)

or

pkg\media\h264reader\nalunittype.go:33:1: calculated cyclomatic complexity for function String is 17, max is 10 (cyclop)
func (n *NalUnitType) String() string {

or

track_local.go:88:1: WriteStream returns interface (github.com/pion/webrtc/v4.TrackLocalWriter) (ireturn)
func (t *baseTrackLocalContext) WriteStream() TrackLocalWriter {

However, most issues are just whitespace or "pretty" lints. I'm unsure whether to make one large PR per repo (addressing all tasks) or break it down into smaller PRs. do we have a merge queue? I'm happy to go with either approach!

[Sean-Der]

Manually modifying and merging sounds great! When we update it in this repo it will be a no-op in all the other repos.

I think doing //nolint for things like time in the example makes sense. I optimize for least cognitive complexity. A constant is only useful if it is used in multiple places AND giving it a name adds more documentation.

Refactoring is fine with me! Whatever makes the code most maintainable is my goal. I think a high cyclomatic complexity is fine if the code is more readable. Lots of small 'helper' functions are more complicated IMO.

I found https://minds.md/zakirullin/cognitive really helpful recently. My goal with Pion is to make it as readable/maintainable as possible. If we have to disable some linters or do //nolint to achieve that it is fine with me!

[joeturki]

That was a good read, Thanks, Noted.

Do we go with a single PR for each Repo, or do I break it down to smaller PRs? (based on the lint type).

[Sean-Der]

What do you think of two PRs?

One with all the easy changes. Whitespace/no functional changes.

One with things you aren't sure about. If you make your branch directly on the Pion repos (you should access) I can adjust/fix things and merge them.

Keep things as easy as possible :)

[joeturki]

Oh, I see this will work.

Thanks!

[joeturki]

Should we add fieldalignment to govet?

I see many issues when I enable it.

Example:

pkg\media\media.go:14:13: fieldalignment: struct with 88 pointer bytes could be 72 (govet)
type Sample struct {

Edit: Ignore all the forced pushes :) I was fixing my commit messages lint (Added a pre-push hook so it doesn't happen again).

[jech]

Should we add fieldalignment to govet?

Perhaps later? It's not like you've got too little on your plate.

[jerry-tao]

personally suggest disabling the following linters:

• wsl: This linter can sometimes conflict with gofumpt, leading to inconsistencies in formatting.
• gochecknoinits: Using init functions is quite common in Go, making this linter less practical.
• ireturn: This linter can create issues with interface implementations, which may hinder code flexibility.
• mnd: It often flags common numbers that may not necessarily indicate a -problem.
• interfacebloat: While the concept is worthwhile, it may not be a realistic concern in many codebases.

[joeturki]

Okay Disabled them, I think I'm good to go with working on the repos now?

Maybe last thing. Should we ignore this or upgrade to go 1.22 and loopvar?

level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar."

[jech]

Should we ignore this or upgrade to go 1.22 and loopvar?

Please don't require recent Go versions without a very good reason. It's extra hassle for everyone.

[joeturki]

Please don't require recent Go versions without a very good reason. It's extra hassle for everyone.

Okay, this makes sense, I think this is good enough for now?!