Kevin,
Thank you for your review!
On 2011-12-03 21:53, Kevin Grittner wrote:
>
> (1) Doc changes:
>
> (a) These contain some unnecessary whitespace changes which make it
> harder to see exactly what changed.
This is probably caused because I used emacs's fill-paragraph (alt+q)
command, after some changes. If this is against policy, I could change
the additions in such a way as to cause minor differences, however I
believe that the benefit of that ends immediately after review.
> (b) There's one point where "cursors" should be possessive
> "cursor's".
>
> (c) I think it would be less confusing to be consistent about
> mentioning "positional" and "named" in the same order each
> time. Mixing it up like that is harder to read, at least for
> me.
Good point, both changed.
> (2) The error for mixing positional and named parameters should be
> moved up. That seems more important than "too many arguments" or
> "provided multiple times" if both are true.
I moved the error up, though I personally tend to believe it doesn't
even need to be an error. There is no technical reason not to allow it.
All the user needs to do is make sure that the combination of named
parameters and the positional ones together are complete and not
overlapping. This is also in line with calling functions, where mixing
named and positional is allowed, as long as the positional arguments are
first. Personally I have no opinion what is best, include the feature or
give an error, and I removed the possibility during the previous commitfest.
> (3) The "-- END ADDITIONAL TESTS" comment shouldn't be added to the
> regression tests.
This is removed, also the -- START ADDITIONAL TESTS, though I kept the
tests between those to comments.
> The way this parses out the named parameters and then builds the
> positional list, with some code from read_sql_construct() repeated in
> read_cursor_args() seems a little bit clumsy to me, but I couldn't
> think of a better way to do it.
Same here.
The new patch is attached.
regards
Yeb Havinga
