Nikola,
[dropping one of the Willys from Cc]
On 4/1/22 00:01, Nikola Šale wrote:
That logic was implemented in this patch.
I believe your reading of the issue is correct and the logic makes sense.
Regarding the commit message:
- Please mention the issue like this: "This resolves GitHub Issue
#1621." at the end of the commit message.
Regarding configuration.txt:
- Please wrap the configuration.txt at 80 characters. Your lines are
*slightly* too long.
- Please sort the converters alphabetically.
- The square brackets in the converter "signature" in configuration.txt
are mismatched.
- In the explanation there should not be square brackets around [<delim>].
- Please use "Example:" instead of "Examples:" as the heading.
- Please indent the examples once more.
Regarding sample.c:
- The documentation states "It 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).
- It's not your fault (you've likely taken this from the concat
converter), but 'max' really should be a 'size_t', not an 'int'. Both
concat and add_item would really benefit from the 'ist' API, though.
- The 'v_flag' variable name could be more useful. How about
'var_available'?
- The spaces in your comments are a bit odd. In the first there are two
spaces between 'and we'. For the second, the second line is indented
too much. For the third a space at the end is missing.
Also, I see some converters have corresponding varnish test files and I'm
not sure whether that must be included in the patch or not.
Yes, please include a test. This is the easiest way to verify that the
converter works as expected (and continues to work as expected; e.g.
when refactoring it to use the 'ist' API).
Best regards
Tim Düsterhus
