Re: pgindent vs. git whitespace check
On Wed, Mar 29, 2023 at 08:26:23PM +0200, Daniel Gustafsson wrote: > > On 29 Mar 2023, at 19:18, Bruce Momjian 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 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
> On 29 Mar 2023, at 19:18, Bruce Momjian wrote: > On Thu, Feb 23, 2023 at 09:36:00AM +0100, Daniel Gustafsson wrote: >>> On 23 Feb 2023, at 05:48, Tom Lane 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
On Thu, Feb 23, 2023 at 09:36:00AM +0100, Daniel Gustafsson wrote: > > On 23 Feb 2023, at 05: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. > > 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 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
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
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
> On 23 Feb 2023, at 05: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. 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
John Naylor writes: > On Thu, Feb 23, 2023 at 5:03 AM Andrew Dunstan 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
On Thu, Feb 23, 2023 at 5:03 AM Andrew Dunstan 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
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
Peter Eisentraut 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
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)
