Nikola,
On 4/1/22 16:08, Nikola Šale wrote:
- The square brackets in the converter "signature" in configuration.txt are
mismatched.
- In the explanation there should not be square brackets around [<delim>].
Actually, there are no square brackets around <delim>:
+add_item(<delim>,[<var>][,<suff>]])
See Willy's response. This was about the one in the description, not in
the "signature".
- The documentation states "It(<suff>) may also be omitted but only if the
<var> is supplied". You should verify this with a 'check' function (you're
currently reusing smp_check_concat)
Since the minimum number of arguments is set to 2, maybe there is no need
to verify it because if both <suff> and <var> are omitted the parser will
complain about missing arguments. So maybe the best solution would be to
remove the "but only if the <var> is supplied" part from the documentation
and still use the smp_check_concat check. Let me know how this sounds to
you.
Documentation and code should be consistent. I won't decide which is the
best solution, I'll leave this up to you.
Generally you should make sure to reject any invalid configuration with
a 'check' function, so that it fails loudly instead of silently. So if
the documentation says that at least one must be non-empty, then the
code should verify this. If the documentation says nothing about this,
then the code does not need to do anything. Not saying anything and not
verifying anything probably is the easier solution :-)
Please place it at the top to fix the alphabetical order, and feel free
to move concat with it as it was obviously misplaced.
Sure, but I guess that would require a separate patch(the concat part) to
keep this one clean and focused on the new feature?
Also, apologies for the cosmetic issues, I will try to take better care.
As Willy already said: Don't worry about your first attempt not being
perfect. Your patch is definitely already looking great (especially
since you followed the advice in CONTRIBUTING regarding the commit
message and not using pull requests):
Best regards
Tim Düsterhus
