Thread
Commits
-
Fix comment indentation and whitespace
- 62d56f6720cd 16.0 landed
-
pgindent vs. git whitespace check
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-02-22T08:17:05Z
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. -
Re: pgindent vs. git whitespace check
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-02-22T14:49:48Z
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) -
Re: pgindent vs. git whitespace check
Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-22T14:52:14Z
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 -
Re: pgindent vs. git whitespace check
Andrew Dunstan <andrew@dunslane.net> — 2023-02-22T22:03:13Z
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
-
Re: pgindent vs. git whitespace check
John Naylor <john.naylor@enterprisedb.com> — 2023-02-23T04:12:56Z
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 EDB: http://www.enterprisedb.com
-
Re: pgindent vs. git whitespace check
Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-23T04:48:49Z
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
-
Re: pgindent vs. git whitespace check
Daniel Gustafsson <daniel@yesql.se> — 2023-02-23T08:36:00Z
> 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
-
Re: pgindent vs. git whitespace check
Andrew Dunstan <andrew@dunslane.net> — 2023-02-23T11:37:17Z
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
-
Re: pgindent vs. git whitespace check
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-02-24T15:03:29Z
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.
-
Re: pgindent vs. git whitespace check
Bruce Momjian <bruce@momjian.us> — 2023-03-29T17:18:30Z
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.
-
Re: pgindent vs. git whitespace check
Daniel Gustafsson <daniel@yesql.se> — 2023-03-29T18:26:23Z
> 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
-
Re: pgindent vs. git whitespace check
Bruce Momjian <bruce@momjian.us> — 2023-03-30T14:48:25Z
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.