Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

On Wed, May 19, 2021 at 3:08 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 2:33 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > On Wed, May 19, 2021 at 2:09 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > parse_subscription_options function has some similar code when
> > > throwing errors [with the only difference in the option]. I feel we
> > > could just use a variable for the option and use it in the error.

I am not sure how much it helps to just refactor this part of the code
alone unless we need to add/change it more. Having said that, this
function is being modified by one of the proposed patches for logical
decoding of 2PC and I noticed that the proposed patch is adding more
parameters to this function which already takes 14 input parameters,
so I suggested refactoring it. See comment 11 in email[1]. See, if
that makes sense to you then we can refactor this function such that
it can be enhanced easily by future patches.

> > > While this has no benefit at all, it saves some LOC and makes the code
> > > look better with lesser ereport(ERROR statements. PSA patch.
> > >
> > > Thoughts?
> >
> > I don't have a strong opinion on this, but the patch should add
> > __translator__ help comment for the error msg.
>
> Is the "/*- translator:" help comment something visible to the user or
> some other tool?
>

We use similar comments at other places. So, it makes sense to retain
the comment as it might help translation tools.

[1] - https://www.postgresql.org/message-id/CAA4eK1Jz64rwLyB6H7Z_SmEDouJ41KN42%3DVkVFp6JTpafJFG8Q%40mail.gmail.com

-- 
With Regards,
Amit Kapila.