Add a FieldSeaparator option to tail_csv plugin #3394
Conversation
| WARNING("tail_csv plugin: The \"%s\" config option needs exactly one " | ||
| "string argument.", | ||
| ci->key); | ||
| return -1; |
There was a problem hiding this comment.
You return -1 here, so reporting should be ERROR.
There was a problem hiding this comment.
Other invalid configuration options are also reported as WARNING and should then for consistency also be changed to ERROR, however, I see some more occurrences of WARNING in other plugins for ignored options, so this looks overall consistent me unless I am missing something.
There was a problem hiding this comment.
Don't look into plugins code, maintainers often do merge without deep analysis.
Look into src/daemon/configfile.c that maintained by @collectd/core-maintainers which are more qualified.
The policy is simple - if function returns error, then message should be ERROR too, otherwise WARNING.
There was a problem hiding this comment.
From what I can see in the code ERROR is always followed by return -1, but there are occurrences in the code where return -1 is preceded by WARNING, so I would say causality goes only in one direction here, e.g.
collectd/src/daemon/configfile.c
Lines 414 to 417 in 6079acc
However, from looking at the actual usages in the
tail_csv.c plugin these should very well be errors and warnings should probably only allowed for backwards compatibility which is not the case here.
| return -1; | ||
| } | ||
|
|
||
| len_opt = strlen(ci->values[0].value.string); |
There was a problem hiding this comment.
Policy ask us to declare variables at first use.
There was a problem hiding this comment.
Thanks for looking at this @rpv-tomsk , the variable has been declared at L282 unless I am missing something here?
There was a problem hiding this comment.
|
@rpv-tomsk Ah OK I see it now. Do you want me to open another pull request to fix these issues. I can mark it as a WIP so that merging is hopefully not done until it has been satisfactorily reviewed. |
This PR adds a new option to tail_csv plugin called
FieldSeparator. The value for this option is a single character. This option is used to define a custom separator. Default will be ',' so the PR is backward compatble.Closes #2893
ChangeLog: tail_csv plugin: Adds a FieldSeparator option