Thread: pgindent vs. git whitespace check
Commit e4602483e95 accidentally introduced a situation where pgindent
disagrees with the git whitespace check. The code is
conn = libpqsrv_connect_params(keywords, values,
/* expand_dbname = */ false,
PG_WAIT_EXTENSION);
where the current source file has 4 spaces before the /*, and the
whitespace check says that that should be a tab.
I think it should actually be 3 spaces, so that the "/*..." lines up
with the "keywords..." and "PG_WAIT..." above and below.
I suppose this isn't going to be a quick fix in pgindent, but if someone
is keeping track, maybe this could be added to the to-consider list.
In the meantime, I suggest we work around this, perhaps by
conn = libpqsrv_connect_params(keywords, values, /* expand_dbname = */ false,
PG_WAIT_EXTENSION);
which appears to be robust for both camps.
On 2023-Feb-22, Peter Eisentraut wrote:
> In the meantime, I suggest we work around this, perhaps by
>
> conn = libpqsrv_connect_params(keywords, values, /* expand_dbname = */ false,
> PG_WAIT_EXTENSION);
I suggest
conn = libpqsrv_connect_params(keywords, values,
false, /* expand_dbname */
PG_WAIT_EXTENSION);
which is what we typically do elsewhere and doesn't go overlength.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> Commit e4602483e95 accidentally introduced a situation where pgindent
> disagrees with the git whitespace check. The code is
> conn = libpqsrv_connect_params(keywords, values,
> /* expand_dbname = */ false,
> PG_WAIT_EXTENSION);
> where the current source file has 4 spaces before the /*, and the
> whitespace check says that that should be a tab.
Hmm, I don't think that's per project style in the first place.
Most places that annotate function arguments do it like
conn = libpqsrv_connect_params(keywords, values,
false, /* expand_dbname */
PG_WAIT_EXTENSION);
pgindent has never been very kind to non-end-of-line comments, and
I'm not excited about working on making it do so. As a thought
experiment, what would happen if we reversed course and started
allowing "//" comments? Naive conversion of this comment could
break the code altogether. (Plenty of programming languages
don't even *have* non-end-of-line comments.)
regards, tom lane
On 2023-02-22 We 09:52, Tom Lane wrote:
pgindent has never been very kind to non-end-of-line comments, and I'm not excited about working on making it do so. As a thought experiment, what would happen if we reversed course and started allowing "//" comments? Naive conversion of this comment could break the code altogether. (Plenty of programming languages don't even *have* non-end-of-line comments.)
I suspect not allowing // is at least a minor annoyance to any new developer we acquire under the age of about 40.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, Feb 23, 2023 at 5:03 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>
> I suspect not allowing // is at least a minor annoyance to any new developer we acquire under the age of about 40.
pgindent changes those to our style, so it's not much of an annoyance if one prefers to type it that way during development.
John Naylor <john.naylor@enterprisedb.com> writes:
> On Thu, Feb 23, 2023 at 5:03 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>> I suspect not allowing // is at least a minor annoyance to any new
>> developer we acquire under the age of about 40.
> pgindent changes those to our style, so it's not much of an annoyance if
> one prefers to type it that way during development.
Right, it's not like we reject patches for that (or at least, we shouldn't
reject patches for any formatting issues that pgindent can fix).
For my own taste, I really don't have any objection to // in isolation --
the problem with it is just that we've got megabytes of code in the other
style. I fear it'd look really ugly to have an intermixture of // and /*
comment styles. Mass conversion of /* to // style would answer that,
but would also create an impossible back-patching problem.
regards, tom lane
> On 23 Feb 2023, at 05:48, Tom Lane <tgl@sss.pgh.pa.us> wrote: > For my own taste, I really don't have any objection to // in isolation -- > the problem with it is just that we've got megabytes of code in the other > style. I fear it'd look really ugly to have an intermixture of // and /* > comment styles. We could use the "use the style of surrounding code (comments)" approach - when changing an existing commented function use the style already present; when adding a net new function a choice can be made (unless we mandate a style). It will still look ugly, but it will be less bad than mixing within the same block. > Mass conversion of /* to // style would answer that, > but would also create an impossible back-patching problem. Yeah, that sounds incredibly invasive. -- Daniel Gustafsson
On 2023-02-22 We 23:48, Tom Lane wrote:
For my own taste, I really don't have any objection to // in isolation -- the problem with it is just that we've got megabytes of code in the other style. I fear it'd look really ugly to have an intermixture of // and /* comment styles.
Maybe, I've seen some mixing elsewhere and it didn't make me shudder. I agree that you probably wouldn't want to mix both styles for end of line comments in a single function, although a rule like that would be hard to enforce mechanically.
Mass conversion of /* to // style would answer that, but would also create an impossible back-patching problem.
Yeah, I agree that's a complete non-starter.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 22.02.23 15:49, Alvaro Herrera wrote: > On 2023-Feb-22, Peter Eisentraut wrote: > >> In the meantime, I suggest we work around this, perhaps by >> >> conn = libpqsrv_connect_params(keywords, values, /* expand_dbname = */ false, >> PG_WAIT_EXTENSION); > > I suggest > > conn = libpqsrv_connect_params(keywords, values, > false, /* expand_dbname */ > PG_WAIT_EXTENSION); > > which is what we typically do elsewhere and doesn't go overlength. Fixed this way.
On Thu, Feb 23, 2023 at 09:36:00AM +0100, Daniel Gustafsson wrote: > > On 23 Feb 2023, at 05:48, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > For my own taste, I really don't have any objection to // in isolation -- > > the problem with it is just that we've got megabytes of code in the other > > style. I fear it'd look really ugly to have an intermixture of // and /* > > comment styles. > > We could use the "use the style of surrounding code (comments)" approach - when > changing an existing commented function use the style already present; when > adding a net new function a choice can be made (unless we mandate a style). It > will still look ugly, but it will be less bad than mixing within the same > block. > > > Mass conversion of /* to // style would answer that, > > but would also create an impossible back-patching problem. > > Yeah, that sounds incredibly invasive. I am replying late here but ... We would have to convert all supported branches, and tell all forks to do the same (hopefully at the same time). The new standard would then be for all single-line comments to use // instead of /* ... */. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
> On 29 Mar 2023, at 19:18, Bruce Momjian <bruce@momjian.us> wrote: > On Thu, Feb 23, 2023 at 09:36:00AM +0100, Daniel Gustafsson wrote: >>> On 23 Feb 2023, at 05:48, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Mass conversion of /* to // style would answer that, >>> but would also create an impossible back-patching problem. >> >> Yeah, that sounds incredibly invasive. > > I am replying late here but ... > > We would have to convert all supported branches, and tell all forks to > do the same (hopefully at the same time). The new standard would then > be for all single-line comments to use // instead of /* ... */. That still leaves every patch which is in flight on -hackers, and conflicts in local development trees etc. It's doable (apart from forks, but that cannot be our core concern), but I personally can't see the price paid justify the result. -- Daniel Gustafsson
On Wed, Mar 29, 2023 at 08:26:23PM +0200, Daniel Gustafsson wrote: > > On 29 Mar 2023, at 19:18, Bruce Momjian <bruce@momjian.us> wrote: > > We would have to convert all supported branches, and tell all forks to > > do the same (hopefully at the same time). The new standard would then > > be for all single-line comments to use // instead of /* ... */. > > That still leaves every patch which is in flight on -hackers, and conflicts in > local development trees etc. It's doable (apart from forks, but that cannot be > our core concern), but I personally can't see the price paid justify the result. Yes, this would have to be done at the start of a new release cycle. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.