On Fri, Apr 21, 2023 at 7:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > The reason I left it like this when reviewing and committing is that I think it
> > makes for more readable code. The amount of lines saved is pretty small, and
> > "shuffle" isn't an exact term so by reading the code it isn't immediate clear
> > what such a function would do. By having the shuffle algorithm where it's used
> > it's clear what the code does and what the outcome is. If others disagree I
> > can go ahead and refactor of course, but I personally would not deem it a net
> > win in code quality.
>
> I think we should avoid nitpicking stuff like this. I likely would
> have used a subroutine if I'd done it myself, but I definitely
> wouldn't have submitted a patch to change whatever the last person did
> without some tangible reason for so doing.
Code duplication, and the possibility that a change in one location
(bugfix or otherwise) does not get applied to the other locations, is
a good enough reason to submit a patch, IMHO.
> It's not a good use of
> reviewer and committer time to litigate things like this.
Postgres has a very high bar for code quality, and for documentation.
It is a major reason that attracts people to, and keeps them in the
Postgres ecosystem, as users and contributors. If anything, we should
encourage folks to point out such inconsistencies in code and docs
that keep the quality high.
This is not a attack on any one commit, or author/committer of the
commit; sorry if it appeared that way. I was merely reviewing the
commit that introduced a nice libpq feature. This patch is merely a
minor improvement to the code that I think deserves a consideration.
It's not a litigation, by any means.
Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com
