Thread

1. Re: [REVIEW] Patch for cursor calling with named parameters

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-03T20:53:43Z

   The patch is in context format, includes docs and additional
   regression tests, applies cleanly, passes "world" regression tests.
   The parser changes don't cause any "backing up".
    
   Discussion on the list seems to have reached a consensus that this
   patch implements a useful feature.  A previous version was reviewed.
   This version attempts to respond to points previously raised.
    
   Overall, it looked good, but there were a few things I would like Yeb
   to address before mark it is marked ready for committer.  I don't
   think any of these will take long.
    
   (1)  Doc changes:
    
     (a) These contain some unnecessary whitespace changes which make it
         harder to see exactly what changed.
    
     (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.
    
   (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.
    
   (3)  The "-- END ADDITIONAL TESTS" comment shouldn't be added to the
   regression tests.
    
   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.
    
   -Kevin
   
   
   

2. Re: [REVIEW] Patch for cursor calling with named parameters

   Yeb Havinga <yebhavinga@gmail.com> — 2011-12-05T10:06:47Z

   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
   
   

3. Re: [REVIEW] Patch for cursor calling with named parameters

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-05T15:34:58Z

   Yeb Havinga <yebhavinga@gmail.com> wrote:
   > 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.
    
   I don't know whether it's a hard policy, but people usually minimize
   whitespace changes in such situations, to make it easier for the
   reviewer and committer to tell what really changed.  The committer
   can always reformat after looking that over, if they feel that's
   useful.  The issue is small enough here that I'm not inclined to
   consider it a blocker for sending to the committer.
    
   >> (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.
    
   If there's no technical reason not to allow them to be mixed, I
   would tend to favor consistency with parameter calling rules.  Doing
   otherwise seems like to result in confusion and "bug" reports.
    
   How do others feel?
    
   -Kevin
   
   

4. Re: [REVIEW] Patch for cursor calling with named parameters

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-05T17:09:06Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
   > Yeb Havinga <yebhavinga@gmail.com> wrote:
    
   >> 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.
   >  
   > If there's no technical reason not to allow them to be mixed, I
   > would tend to favor consistency with parameter calling rules. 
   > Doing otherwise seems like to result in confusion and "bug"
   > reports.
    
   Er, that was supposed to read "I would tend to favor consistency
   with function calling rules."  As stated here:
    
   http://www.postgresql.org/docs/9.1/interactive/sql-syntax-calling-funcs.html
    
   | PostgreSQL also supports mixed notation, which combines positional
   | and named notation. In this case, positional parameters are
   | written first and named parameters appear after them.
    
   > How do others feel?
    
   If there are no objections, I suggest that Yeb implement the mixed
   notation for cursor parameters.
    
   -Kevin
   
   

5. Re: [REVIEW] Patch for cursor calling with named parameters

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-06T16:58:53Z

   Kevin Grittner <kgrittn@wicourts.gov> wrote:
   > Yeb Havinga <yebhavinga@gmail.com> wrote:
    
   >> 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.
    
   > If there are no objections, I suggest that Yeb implement the mixed
   > notation for cursor parameters.
    
   Hearing no objections -- Yeb, are you OK with doing this, and do you
   feel this is doable for this CF?
    
   -Kevin
   
   

6. Re: [REVIEW] Patch for cursor calling with named parameters

   Yeb Havinga <yebhavinga@gmail.com> — 2011-12-07T09:00:25Z

   On 2011-12-06 17:58, Kevin Grittner wrote:
   > Kevin Grittner<kgrittn@wicourts.gov>  wrote:
   >> If there are no objections, I suggest that Yeb implement the mixed
   >> notation for cursor parameters.
   >
   > Hearing no objections -- Yeb, are you OK with doing this, and do you
   > feel this is doable for this CF?
   
   It is not a large change, I'll be able to do it tomorrow if nothing 
   unexpected happens, or monday otherwise.
   
   regards,
   Yeb
   
   
   

7. Re: [REVIEW] Patch for cursor calling with named parameters

   Yeb Havinga <yebhavinga@gmail.com> — 2011-12-11T15:26:33Z

   On 2011-12-06 17:58, Kevin Grittner wrote:
   > Kevin Grittner<kgrittn@wicourts.gov>  wrote:
   >> Yeb Havinga<yebhavinga@gmail.com>  wrote:
   >
   >>> 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.
   >
   >> If there are no objections, I suggest that Yeb implement the mixed
   >> notation for cursor parameters.
   >
   > Hearing no objections -- Yeb, are you OK with doing this, and do you
   > feel this is doable for this CF?
   
   Attach is v6 of the patch, allowing mixed mode and with new test cases 
   in the regression tests. One difference with calling functions remains: 
   it is allowed to place positional arguments after named parameters. 
   Including that would add code, but nothing would be gained.
   
   regards,
   Yeb Havinga
   
   
   

8. Re: [REVIEW] Patch for cursor calling with named parameters

   Yeb Havinga <yebhavinga@gmail.com> — 2011-12-12T12:32:41Z

   On 2011-12-11 16:26, Yeb Havinga wrote:
   > On 2011-12-06 17:58, Kevin Grittner wrote:
   >> Kevin Grittner<kgrittn@wicourts.gov>  wrote:
   >>> Yeb Havinga<yebhavinga@gmail.com>  wrote:
   >>
   >>>> 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.
   >>
   >>> If there are no objections, I suggest that Yeb implement the mixed
   >>> notation for cursor parameters.
   >>
   >> Hearing no objections -- Yeb, are you OK with doing this, and do you
   >> feel this is doable for this CF?
   >
   > Attach is v6 of the patch, allowing mixed mode and with new test cases 
   > in the regression tests. One difference with calling functions 
   > remains: it is allowed to place positional arguments after named 
   > parameters. Including that would add code, but nothing would be gained.
   
   Forgot to copy regression output to expected - attached v7 fixes that.
   
   -- Yeb