chore: Fix linter findings for `revive:exported` in `plugins/inputs/[a-b]*` #15913

zak-pawel · 2024-09-19T11:07:36Z

Summary

Address findings for revive:exported in plugins/inputs/[a-b]*.

As part of this effort for files from plugins/inputs/[a-b]*, the following actions were taken:

Type names (const, var, struct, func, etc) were changed to unexported, wherever they didn't need to be exported.
All remaining exported types were given comments in the appropriate form – this does not apply to exported methods that implement "known" plugin interfaces (Gather|Init|Start|Stop|SampleConfig|Parse|Add|Apply|Serialize|SerializeBatch|SetParser).
The order of methods was organized (exported methods first, then unexported, with init at the very end).

It is only part of the bigger job (for issue: #15813).
After all findings of this type in whole project are handled, we can enable revive:exported rule in golangci-lint.

Checklist

No AI generated code was used in this PR

…a-b]*`

srebhan

Thanks for your effort @zak-pawel! I do have some comments with the most sever being to remove the useless comments! :-)

srebhan · 2024-09-20T15:39:50Z

plugins/inputs/activemq/activemq.go

@@ -22,6 +22,7 @@ import (
 //go:embed sample.conf
 var sampleConfig string

+// ActiveMQ Input Plugin


Please don't add these nonsense comments!

srebhan · 2024-09-20T15:41:05Z

plugins/inputs/aerospike/aerospike.go

@@ -21,6 +21,7 @@ import (
 //go:embed sample.conf
 var sampleConfig string

+// Aerospike Input Plugin


Same here (and potentially everywhere else ;-)) What information does this comment add?

srebhan · 2024-09-20T15:44:06Z

plugins/inputs/amqp_consumer/amqp_consumer.go

-type externalAuth struct{}
-
+// Mechanism returns "EXTERNAL"
 func (a *externalAuth) Mechanism() string {
 	return "EXTERNAL"
 }
+
+// Response returns "\000"
 func (a *externalAuth) Response() string {
 	return "\000"
 }


Please revert this! Keep the struct close to the method definition of that struct and remove those comments telling you exactly the same as the code does!

srebhan · 2024-09-20T15:45:15Z

plugins/inputs/aurora/aurora.go

@@ -207,7 +208,7 @@ func (a *Aurora) gatherScheduler(
 		return fmt.Errorf("%v", resp.Status)
 	}

-	var vars Vars
+	var vars vars


Ouch... This really hurts my eyes. :-)

I think it's beautiful ;)

zak-pawel · 2024-09-20T17:29:11Z

I do have some comments with the most sever being to remove the useless comments! :-)

@srebhan
The thing is, the exported rule is designed to detect uncommented "things" of the exported type. And while we can filter out methods with known names, it will be hard for us to do that for several hundred plugins and their public structs.

The options I see are:

Grit our teeth and put a comment like // StructureName over every public structure (representing a certain plugin) everywhere (we don't need anything more).
As above, but put //nolint xD
Did you know that these structs don't have to be exported? We can make them unexported... Only their fields need to be exported.
We can take the same steps I took in this PR for the entire codebase (except for the structs we're talking about), but not enable the exported rule.

Point 2 seems very poor.
Similarly, point 4 - because I see value in this rule - it will make it easier for us to review new code, in which we often overlook that there are types that could be unexported.

srebhan · 2024-09-30T12:17:57Z

@zak-pawel how about adding a config option to the linter? :-)

zak-pawel · 2024-09-30T12:22:51Z

@zak-pawel how about adding a config option to the linter? :-)

@srebhan To disable checking whether comments exist on the exported structs?

srebhan · 2024-09-30T17:09:06Z

Yes. We just need a PR on revive for that. :-P

zak-pawel · 2024-09-30T22:05:28Z

Yes. We just need a PR on revive for that. :-P

@srebhan Such PR already exists: mgechev/revive#1047

…nputs_a-b

…nputs_a-b # Conflicts: # plugins/inputs/beat/beat.go

…nputs_a-b

telegraf-tiger · 2024-10-04T06:03:00Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

👍 This pull request doesn't change the Telegraf binary size

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

chore: Fix linter findings for revive:exported in `plugins/inputs/[…

f7d0dc7

…a-b]*`

telegraf-tiger bot added the chore label Sep 19, 2024

zak-pawel requested review from srebhan and DStrand1 September 19, 2024 11:07

zak-pawel assigned srebhan and DStrand1 Sep 19, 2024

zak-pawel added the linter label Sep 19, 2024

srebhan reviewed Sep 20, 2024

View reviewed changes

srebhan unassigned DStrand1 Sep 20, 2024

zak-pawel added 4 commits October 3, 2024 19:57

Merge remote-tracking branch 'upstream/master' into revive-exported-i…

4bcebd7

…nputs_a-b

Merge remote-tracking branch 'upstream/master' into revive-exported-i…

3ce56e2

…nputs_a-b # Conflicts: # plugins/inputs/beat/beat.go

remove nonsense :(

f4ba6a9

Merge remote-tracking branch 'upstream/master' into revive-exported-i…

b9efeb5

…nputs_a-b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Fix linter findings for `revive:exported` in `plugins/inputs/[a-b]*` #15913

chore: Fix linter findings for `revive:exported` in `plugins/inputs/[a-b]*` #15913

zak-pawel commented Sep 19, 2024

srebhan left a comment

srebhan Sep 20, 2024

zak-pawel Oct 3, 2024

srebhan Sep 20, 2024

zak-pawel Oct 3, 2024

srebhan Sep 20, 2024

zak-pawel Oct 3, 2024

srebhan Sep 20, 2024

zak-pawel Oct 3, 2024

zak-pawel commented Sep 20, 2024

srebhan commented Sep 30, 2024

zak-pawel commented Sep 30, 2024 •

edited

Loading

srebhan commented Sep 30, 2024

zak-pawel commented Sep 30, 2024

telegraf-tiger bot commented Oct 4, 2024

Artifact URLs

chore: Fix linter findings for revive:exported in plugins/inputs/[a-b]* #15913

Are you sure you want to change the base?

chore: Fix linter findings for revive:exported in plugins/inputs/[a-b]* #15913

Conversation

zak-pawel commented Sep 19, 2024

Summary

Checklist

srebhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zak-pawel commented Sep 20, 2024

srebhan commented Sep 30, 2024

zak-pawel commented Sep 30, 2024 • edited Loading

srebhan commented Sep 30, 2024

zak-pawel commented Sep 30, 2024

telegraf-tiger bot commented Oct 4, 2024

Artifact URLs

chore: Fix linter findings for `revive:exported` in `plugins/inputs/[a-b]*` #15913

chore: Fix linter findings for `revive:exported` in `plugins/inputs/[a-b]*` #15913

zak-pawel commented Sep 30, 2024 •

edited

Loading