On Thu, 5 Jan 2023 at 18:22, Dean Rasheed <dean.a.rash...@gmail.com> wrote: > > On Tue, 6 Dec 2022 at 19:12, vignesh C <vignes...@gmail.com> wrote: > > > > On Tue, 6 Dec 2022 at 20:42, Melih Mutlu <m.melihmu...@gmail.com> wrote: > > > > > > Also one little suggestion: > > > > > >> + if (ends_with(prev_wd, ')')) > > >> + COMPLETE_WITH(Alter_routine_options, "CALLED ON NULL INPUT", > > >> + "RETURNS NULL ON NULL INPUT", "STRICT", "SUPPORT"); > > > > > > What do you think about gathering FUNCTION options as you did with > > > ROUTINE options. > > > Something like the following would seem nicer, I think. > > > > > >> #define Alter_function_options \ > > >> Alter_routine_options, "CALLED ON NULL INPUT", \ > > >> "RETURNS NULL ON NULL INPUT", "STRICT", "SUPPORT" > > > > I did not make it as a macro for alter function options as it is used > > only in one place whereas the others were required in more than one > > place. > > My feeling is that having this macro somewhat improves readability and > consistency between the 3 cases, so I think it's worth it, even if > it's only used once. > > I think it slightly improves readability to keep all the arguments to > Matches() on one line, and that seems to be the style elsewhere, even > if that makes the line longer than 80 characters. > > Also in the interests of readability, I think it's slightly easier to > follow if the "ALTER PROCEDURE <name> (...)" and "ALTER ROUTINE <name> > (...)" cases are made to immediately follow the "ALTER FUNCTION <name> > (...)" case, with the longer/more complex cases following on from > that. > > That leads to the attached, which barring objections, I'll push shortly.
The changes look good to me. Regards, Vignesh