Thread: pgindent
I think it's about time for us to run pgindent. I did a trial run today of pgindent today and came up with the attached patch for typedefs.list, which I'd like to commit more or less immediately, barring objections. It mostly just adds new typedefs that have appeared over the last year, but it also realphabetizes the file - some things that were added incrementally seem to have ended up in what is, at least according to what sort likes to do on my machine, the wrong place in the file. With this applied, I get a fairly clean pgindent run. There are some problems with comments getting mangled, and in a couple of cases function definitions getting mangled, that need more investigation. I'll try to find time to look into that soon and follow up, unless somebody else beats me to it. As far as possible, I think it's desirable to clean up those things before rather than after running pgindent, because unmangling ASCII art that pgindent has stepped on is a thankless chore. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Wed, Apr 27, 2016 at 10:51:55AM -0400, Robert Haas wrote: > I think it's about time for us to run pgindent. I did a trial run > today of pgindent today and came up with the attached patch for > typedefs.list, which I'd like to commit more or less immediately, > barring objections. It mostly just adds new typedefs that have > appeared over the last year, but it also realphabetizes the file - > some things that were added incrementally seem to have ended up in > what is, at least according to what sort likes to do on my machine, > the wrong place in the file. > > With this applied, I get a fairly clean pgindent run. There are some > problems with comments getting mangled, and in a couple of cases > function definitions getting mangled, that need more investigation. > I'll try to find time to look into that soon and follow up, unless > somebody else beats me to it. As far as possible, I think it's > desirable to clean up those things before rather than after running > pgindent, because unmangling ASCII art that pgindent has stepped on is > a thankless chore. Agreed. Sounds like a good plan --- a better plan than I have used in the past. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On 2016-04-27 10:51:55 -0400, Robert Haas wrote: > I think it's about time for us to run pgindent. Sounds reasonable. > I did a trial run > today of pgindent today and came up with the attached patch for > typedefs.list, which I'd like to commit more or less immediately, > barring objections. Yes, that makes sense. That way other can easily look at "their" code, to see whether it can be made more pgindent resistant ;) > It mostly just adds new typedefs that have > appeared over the last year, but it also realphabetizes the file - > some things that were added incrementally seem to have ended up in > what is, at least according to what sort likes to do on my machine, > the wrong place in the file. Is it just me, or is the sort order in that file a bit confusing? The whole thing about upper and lower case being separated seems to make it much harder than necessary to manually insert something in the right place.. - Andres
On Wed, Apr 27, 2016 at 11:45 AM, Andres Freund <andres@anarazel.de> wrote: > Yes, that makes sense. That way other can easily look at "their" code, > to see whether it can be made more pgindent resistant ;) Right. >> It mostly just adds new typedefs that have >> appeared over the last year, but it also realphabetizes the file - >> some things that were added incrementally seem to have ended up in >> what is, at least according to what sort likes to do on my machine, >> the wrong place in the file. > > Is it just me, or is the sort order in that file a bit confusing? The > whole thing about upper and lower case being separated seems to make it > much harder than necessary to manually insert something in the right > place.. Except for recently-manually-added entries, it seems to match what sort wants to do on my system exactly. Which seems good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 27, 2016 at 10:57 AM, Bruce Momjian <bruce@momjian.us> wrote: > Agreed. Sounds like a good plan --- a better plan than I have used in > the past. Thinking about this a bit more, what I am inclined to do is: 1. Run pgindent. 2. Read the diff and revert any changes that look icky. 3. Commit the rest. 4. Run pgindent again and post the diff. Discuss how to fix the ickiness contained therein, then proceed accordingly. How does that sound? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 27, 2016 at 11:51:42AM -0400, Robert Haas wrote: > >> It mostly just adds new typedefs that have > >> appeared over the last year, but it also realphabetizes the file - > >> some things that were added incrementally seem to have ended up in > >> what is, at least according to what sort likes to do on my machine, > >> the wrong place in the file. > > > > Is it just me, or is the sort order in that file a bit confusing? The > > whole thing about upper and lower case being separated seems to make it > > much harder than necessary to manually insert something in the right > > place.. > > Except for recently-manually-added entries, it seems to match what > sort wants to do on my system exactly. Which seems good. Well, sort ordering is all related to your collation setting: $ echo "a> A" | LC_COLLATE="" sortaA$ echo "aA" | LC_COLLATE="en_US" sortAa -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Wed, Apr 27, 2016 at 11:57 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Wed, Apr 27, 2016 at 11:51:42AM -0400, Robert Haas wrote: >> >> It mostly just adds new typedefs that have >> >> appeared over the last year, but it also realphabetizes the file - >> >> some things that were added incrementally seem to have ended up in >> >> what is, at least according to what sort likes to do on my machine, >> >> the wrong place in the file. >> > >> > Is it just me, or is the sort order in that file a bit confusing? The >> > whole thing about upper and lower case being separated seems to make it >> > much harder than necessary to manually insert something in the right >> > place.. >> >> Except for recently-manually-added entries, it seems to match what >> sort wants to do on my system exactly. Which seems good. > > Well, sort ordering is all related to your collation setting: > > $ echo "a > > A" | LC_COLLATE="" sort > a > A > > $ echo "a > A" | LC_COLLATE="en_US" sort > A > a Sure. I guess we could resort that file with LC_COLLATE=C. But my point was mostly just that the ordering is hardly haphazard. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I think it's about time for us to run pgindent. I did a trial run
> today of pgindent today and came up with the attached patch for
> typedefs.list, which I'd like to commit more or less immediately,
> barring objections.
Um, we normally take the buildfarm's list of typedefs, not anything
manually created.
regards, tom lane
On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I think it's about time for us to run pgindent. I did a trial run >> today of pgindent today and came up with the attached patch for >> typedefs.list, which I'd like to commit more or less immediately, >> barring objections. > > Um, we normally take the buildfarm's list of typedefs, not anything > manually created. Well, we can still do that, but I don't see much advantage in it. It just churns the file to the extent that manual review of the changes is impossible, and then when pgindent does the wrong thing it only gets reported after the fact. How is that better than making sure that the contents of the file are such as to actually produce good output from pgindent? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 27, 2016 at 02:54:35PM -0400, Robert Haas wrote: > On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> I think it's about time for us to run pgindent. I did a trial run > >> today of pgindent today and came up with the attached patch for > >> typedefs.list, which I'd like to commit more or less immediately, > >> barring objections. > > > > Um, we normally take the buildfarm's list of typedefs, not anything > > manually created. > > Well, we can still do that, but I don't see much advantage in it. It > just churns the file to the extent that manual review of the changes > is impossible, and then when pgindent does the wrong thing it only > gets reported after the fact. How is that better than making sure > that the contents of the file are such as to actually produce good > output from pgindent? Using the buildfarm typedefs assures they are always generated in a consistent way. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Wed, Apr 27, 2016 at 05:01:14PM -0400, Bruce Momjian wrote: > On Wed, Apr 27, 2016 at 02:54:35PM -0400, Robert Haas wrote: > > On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Robert Haas <robertmhaas@gmail.com> writes: > > >> I think it's about time for us to run pgindent. I did a trial run > > >> today of pgindent today and came up with the attached patch for > > >> typedefs.list, which I'd like to commit more or less immediately, > > >> barring objections. > > > > > > Um, we normally take the buildfarm's list of typedefs, not anything > > > manually created. > > > > Well, we can still do that, but I don't see much advantage in it. It > > just churns the file to the extent that manual review of the changes > > is impossible, and then when pgindent does the wrong thing it only > > gets reported after the fact. How is that better than making sure > > that the contents of the file are such as to actually produce good > > output from pgindent? > > Using the buildfarm typedefs assures they are always generated in a > consistent way. Oh, and as I remember the buildfarm merges several platforms, including Windows, to make that list, so I suggest you use that one. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Um, we normally take the buildfarm's list of typedefs, not anything
>> manually created.
> Well, we can still do that, but I don't see much advantage in it. It
> just churns the file to the extent that manual review of the changes
> is impossible, and then when pgindent does the wrong thing it only
> gets reported after the fact. How is that better than making sure
> that the contents of the file are such as to actually produce good
> output from pgindent?
On what grounds do you claim the buildfarm result is unstable?
I've been using that for a long time and it works fine. Moreover,
ignoring that data is a bad idea because it reflects platform-specific
variations in the set of typedefs that are known. If you build a
typedefs list based only on what works on your machine, it likely
won't work for other people.
regards, tom lane
On Wed, Apr 27, 2016 at 6:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Apr 27, 2016 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Um, we normally take the buildfarm's list of typedefs, not anything >>> manually created. > >> Well, we can still do that, but I don't see much advantage in it. It >> just churns the file to the extent that manual review of the changes >> is impossible, and then when pgindent does the wrong thing it only >> gets reported after the fact. How is that better than making sure >> that the contents of the file are such as to actually produce good >> output from pgindent? > > On what grounds do you claim the buildfarm result is unstable? > I've been using that for a long time and it works fine. Moreover, > ignoring that data is a bad idea because it reflects platform-specific > variations in the set of typedefs that are known. If you build a > typedefs list based only on what works on your machine, it likely > won't work for other people. /me shrugs Well, let's get the list, then, and compare it to what's in the file now. How do we do that exactly? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 27, 2016 at 11:38:57PM -0400, Robert Haas wrote:
> > On what grounds do you claim the buildfarm result is unstable?
> > I've been using that for a long time and it works fine. Moreover,
> > ignoring that data is a bad idea because it reflects platform-specific
> > variations in the set of typedefs that are known. If you build a
> > typedefs list based only on what works on your machine, it likely
> > won't work for other people.
>
> /me shrugs
>
> Well, let's get the list, then, and compare it to what's in the file
> now. How do we do that exactly?
The URL is in the file src/tools/pgindent/README:
5) Download the typedef file from the buildfarm:
wget -O src/tools/pgindent/typedefs.list http://buildfarm.postgresql.org/cgi-bin/typedefs.pl
(see http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list for a full list of typedefs, also
http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html)
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
On Thu, Apr 28, 2016 at 7:39 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Wed, Apr 27, 2016 at 11:38:57PM -0400, Robert Haas wrote: >> > On what grounds do you claim the buildfarm result is unstable? >> > I've been using that for a long time and it works fine. Moreover, >> > ignoring that data is a bad idea because it reflects platform-specific >> > variations in the set of typedefs that are known. If you build a >> > typedefs list based only on what works on your machine, it likely >> > won't work for other people. >> >> /me shrugs >> >> Well, let's get the list, then, and compare it to what's in the file >> now. How do we do that exactly? > > The URL is in the file src/tools/pgindent/README: > > 5) Download the typedef file from the buildfarm: > > wget -O src/tools/pgindent/typedefs.list http://buildfarm.postgresql.org/cgi-bin/typedefs.pl > > (see http://www.pgbuildfarm.org/cgi-bin/typedefs.pl?show_list for a full list of typedefs, > also http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html) I compared the result of running pgindent with the typedefs.list file as updated by me manually with the result of running pgindent using the buildfarm list and ... the buildfarm list is better. Shows what I know. Should we go ahead and commit the current version of that file as src/tools/pgindent/typedefs.list, then? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I compared the result of running pgindent with the typedefs.list file
> as updated by me manually with the result of running pgindent using
> the buildfarm list and ... the buildfarm list is better. Shows what I
> know. Should we go ahead and commit the current version of that file
> as src/tools/pgindent/typedefs.list, then?
Yes, if it's what you plan to use for pgindent'ing.
regards, tom lane