rule.exported: add feature to disable selected warnings for rule (const,method,function)

[mfederowicz]

related with #1045

for rule.exported add feature to read array/list of types which we want to ignore in processing time

[rule.exported]                                                                                                                                                            
  ▎ arguments = [
"checkPrivateReceivers", 
["function","method","type","var"]
]

of course thouse are types discovered when rule.exported is enabled in config, if we set some other additional flags outside of that list it can results with panic/error

[zak-pawel]

It would be very nice to see this feature merged. I would especially need turning off warnings for types.

[mfederowicz]

hi @denisvmedia I resolved conflicts from master, and if you have and sugestions (changes) let me know :)

[chavacava]

Hi @zak-pawel could you please elaborate on the reasons for turning off warnings for types? (I've quickly read the thread on telegraph repo)

[chavacava]

I've left some comments on the implementation details but, in general, IMO the semantics of the new configuration is not clear or, not rightly implemented.
For example, from the configuration documentation we understand that setting "type" will avoid generating any warning on types at all. Then, by reading the implementation we can see that some warning on types will be generated (those of the form "exported type %v should have comment or be unexported")

[chavacava]

the configuration entry gives no clue about the "ignore" nature of the list.

[mfederowicz]

ok i try struct with dedicated fields but in toml there is problem with notation like:

IgnoredWarnings{field:true,field2:true}

you can only use like this:

{field:true,field2:true} or list ["val","val2","val3"]

but there is no option to name/prefix that (or maybe I dont know the magic yet :()

[srebhan]

I think what @chavacava is asking is to have a better naming of the setting, e.g. disableFunctionDocumentationCheck instead of function...

[mfederowicz]

I think what @chavacava is asking is to have a better naming of the setting, e.g. disableFunctionDocumentationCheck instead of function...

ok so each element of ["function","method","type","var"] as dedicated flag

arguments = ["disableFunctionCheck", "disableMethodCheck", "disableTypeCheck", "disableVarCheck"]

that was my first idea, but thinks using array/slice will be simpler (one place to modify) :)

[mfederowicz]

ok maybe we can walk that way:

arguments = ["disableFunctionCheck", "disableMethodCheck", "disableTypeCheck", "disableVarCheck"]

and of course modify my commited code to use that flags insted of additional array in config what you think @chavacava ?

[srebhan]

@chavacava let me elaborate a bit on

Hi @zak-pawel could you please elaborate on the reasons for turning off warnings for types? (I've quickly read the thread on telegraph repo)

Telegraf has a "plugin-like" structure, where each "plugin" fulfills a set of interfaces and by now we exported the plugin struct as well as the interface functions by convention. We now want to use the checker to make sure each plugin only exports the plugin type and the interface functions! However, each of the plugin types has to implement certain functions like Gather() for inputs and we now do have 200+ plugins all implementing Gather(). Guess what the comment for this function is in 99.9999% of all cases? // Gather or Gather gathers data. There is no value in adding these kind of comments. Similar for the plugin struct... Currently, we can only keep the exported rule disabled or add churn to our 300+ plugins... Therefore we would prefer to disable those checks while keeping the ability to impose the convention of only exporting the plugin struct and the interface functions. :-)

[chavacava]

Hi @srebhan thank you for the details on your use case.
As I understand, the "zero-value added" comments problem concerns the code related with plugins. If my understanding is right you ask for the ability to disabling certain warnings of the rule. By doing that you will mute unnecessary warnings on plugins code but... it will also disable warnings on all the code base (and, for example, it will not warn on malformed comments or undocumented public methods). Is that what you need?
Is disabling the rule as per (plugin) file not an option?

[srebhan]

@chavacava we are currently solely checking the plugins with this rule. Disabling the whole rule defeats the purpose as we actually want the exported rule to warn about exported structs and functions only whitelisting the known interface functions. This way the rule will warn about any unexpected exports which is exactly what we want. We only need to shut-up the comment warnings for the plugins...

[chavacava]

@zak-pawel @srebhan
I've made some refactoring in the PR, please check if the rule+configuration behave as you expect for your use case

cc: @mfederowicz

[zak-pawel]

@chavacava I tested it on the Telegraf code, and it looks like the individual switches correctly disable the respective checks. Thanks!

[chavacava]

Thanks for the PR @mfederowicz

[mfederowicz]

@chavacava no problem, but forgot to add var testdata, created quick update: #1055 :P