Hi Nikola,
On Fri, Apr 01, 2022 at 04:08:20PM +0200, Nikola Sale wrote:
> Hi,
>
> Thank you Tim and Willy for your quick and detailed feedback. I'll continue
> working on the patch at the earliest convenience.
OK!
> In the meantime, I'd like to further discuss some points:
>
> - 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>]])
Tim was likely speaking about this one that slipped in the text:
"The first one, [<delim>], ..."
> but I definitely wasn't sure how to define it since both <var> and <suff>
> arguments are optional but at least one has to be specified.
I think your description reads fine by itself. The second argument is
mandatory but may be empty, as such:
- delim is needed
- a first comma is needed
- <var> is optionnal (hence may be empty)
- a second comma is optionnal
- the last field is needed if the comma is written.
That follows the common usages that we have and that a number of programs
usually adopt. And anyway that type of advanced description is also why
there's a full-text description behind ;-)
> That's because
> a minimum number of arguments is set to 2 as less than that wouldn't make
> sense. The way the parser works the following configs work(no parser
> errors):
>
> add_item(",",), add_item(",",,suff), add_item(",",,)
>
>
> but ofc. only in the second case <delim> and <suff> are added to the input
> and in the first and third cases nothing is added as both <var> and <suff>
> are "empty".
That makes sense to me.
> This leads us to the next point :
>
> - 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.
I tend to think that would work. Usually it's important to try to be
as progressive as possible in the description. For example, saying that
"an argument may be omitted if the other one is supplied" is difficult
to think about. Instead you could proceed differently by saying that
"<var> is optional and may optionally be followed by <suff>, however if
<var> is omitted, then <suff> is mandatory".
One thing that makes me doubt about your proposal above of just using the
argument count is that an empty argument is an argument so the count will
be OK if you have "add_item(blah,)" since it contains an optional but
empty argument at the 2nd position. It's true that smp_check_concat()
will check that it does contain a variable and will complain in such a
case though. Thus the check could be:
- if there's a 3rd arg => OK
- else if there's a non-empty 2nd arg that resolves as a variable => OK
- otherwise error
I'm starting to wonder if we're not cutting hair in 4. After all do we
*really* care if someone purposely appends an empty variable AND omits
the suffix ? OK it will not do anything useful, but do we really care ?
I'm more concerned about making sure the doc is understood than about
trying to prevent useless but harmless configurations from being parsed.
Or there's something you can do, which is to enumerate the 3 supported
forms in the doc:
The add_item() converter supports 3 forms:
- add_item(delim,var)
- add_item(delim,var,suffix)
- add_item(delim,,suffix)
And for each you can give a quick description of what it does because it
should be very simple since the logic is simple as well.
> 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?
It's no big deal for this one, feel free to move it with it, as it will
not be backported anyway and has no user-visible effect. It happened to
us a few times already, just like when we have to re-align some fields
to accommodate a larger function name.
> Also, apologies for the cosmetic issues, I will try to take better care.
No worries, that's unavoidable and it's a good sign when the first
comments are only about this ;-)
Willy
