Обсуждение: Shapes on the regression test for polygon
The first two shapes on src/test/regress/sql/polygon.sql do not make sense to me. They look more like polygons with some more tabs, but still did not match the coordinates. I changed them to make consistent with the shapes. I believe this was the intention of the original author. Patch attached.
Вложения
On Mon, Jul 21, 2014 at 10:52 AM, Emre Hasegeli <emre@hasegeli.com> wrote: > The first two shapes on src/test/regress/sql/polygon.sql do not make > sense to me. They look more like polygons with some more tabs, > but still did not match the coordinates. I changed them to make > consistent with the shapes. I believe this was the intention of > the original author. Patch attached. Well, I think the number of tabs that makes them look best depends on your tab-stop setting. At present, I find that with 8-space tabs things seem to line up pretty well, whereas with your patch, 4-space tabs line up well. Either way, I have no idea what the picture is supposed to mean, because it looks to me like the original picture has circles at (3,3) and (2,1) and plus signs at (2,2), (3,1), and (4,0). With your patch applied, the circle at (2,1) has moved to (2,0.5). I can't correlate any of this to the test cases you modified (and which I see no reason to modify, whether they match the picture or not). And really, if the diagram is confusing, let's just remove it. We don't really need our regression test comments to illustrate what a triangle looks like, or how to do bad ASCII art. Personally, though, my vote would be to just leave it the way it is. I don't think there's really a problem here in need of solving. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 21, 2014 at 10:52 AM, Emre Hasegeli <emre@hasegeli.com> wrote:
>> The first two shapes on src/test/regress/sql/polygon.sql do not make
>> sense to me.
> Well, I think the number of tabs that makes them look best depends on
> your tab-stop setting. At present, I find that with 8-space tabs
> things seem to line up pretty well, whereas with your patch, 4-space
> tabs line up well.
Perhaps we should expand tabs to spaces in those ascii-art diagrams?
> Personally, though, my vote would be to just leave it the way it is.
> I don't think there's really a problem here in need of solving.
I concur with doubting that changing the actual regression test cases
is a good thing to do, at least not without more analysis. But "the
pictures make no sense with the wrong tab setting" is a pretty simple
issue, and one that I can see biting people.
regards, tom lane
On Wed, Jul 23, 2014 at 4:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jul 21, 2014 at 10:52 AM, Emre Hasegeli <emre@hasegeli.com> wrote: >>> The first two shapes on src/test/regress/sql/polygon.sql do not make >>> sense to me. > >> Well, I think the number of tabs that makes them look best depends on >> your tab-stop setting. At present, I find that with 8-space tabs >> things seem to line up pretty well, whereas with your patch, 4-space >> tabs line up well. > > Perhaps we should expand tabs to spaces in those ascii-art diagrams? > >> Personally, though, my vote would be to just leave it the way it is. >> I don't think there's really a problem here in need of solving. > > I concur with doubting that changing the actual regression test cases > is a good thing to do, at least not without more analysis. But "the > pictures make no sense with the wrong tab setting" is a pretty simple > issue, and one that I can see biting people. AFAICT, the pictures make no sense with the right tab setting, either. The possibility that someone might use the wrong tab setting is just icing on the cake. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> Well, I think the number of tabs that makes them look best depends on > your tab-stop setting. At present, I find that with 8-space tabs > things seem to line up pretty well, whereas with your patch, 4-space > tabs line up well. Either way, I have no idea what the picture is > supposed to mean, because it looks to me like the original picture has > circles at (3,3) and (2,1) and plus signs at (2,2), (3,1), and (4,0). > With your patch applied, the circle at (2,1) has moved to (2,0.5). I > can't correlate any of this to the test cases you modified (and which > I see no reason to modify, whether they match the picture or not). 4 space tab-stop is not the project standard? The circle I moved does not represent a corner of the polygon. I just moved it to make the line straight after the tabs. > And really, if the diagram is confusing, let's just remove it. We > don't really need our regression test comments to illustrate what a > triangle looks like, or how to do bad ASCII art. > > Personally, though, my vote would be to just leave it the way it is. > I don't think there's really a problem here in need of solving. I am sorry for taking your time for such a low priority problem, but as we stumble across this, let me to make the change more clear. According to git blame the tests with the shapes added by 04688df6. The coordinates was written in the old format like this: (3.0,3.0,1.0,1.0,3.0,0.0) These are changed by 3d9584c9 to the new format like this: (3.0,1.0),(3.0,3.0),(1.0,0.0) In my opinion, the real intention of the first commit was to write them in the new format like this: (3.0,3.0),(1.0,1.0),(3.0,0.0) It makes sense because the corners match the shape. So, I changed it that way. If we will not going to change the tests, It would be okay to just remove the shapes. I do not think they add more value to the tests.
Emre Hasegeli <emre@hasegeli.com> writes:
>> Well, I think the number of tabs that makes them look best depends on
>> your tab-stop setting. At present, I find that with 8-space tabs
>> things seem to line up pretty well, whereas with your patch, 4-space
>> tabs line up well.
> 4 space tab-stop is not the project standard?
It is for C code, but there's less agreement about non-code files
(and no enforcement mechanism like pgindent, either). People may or
may not have their editors configured to do the right thing in non-C
files, so it seems best to avoid cases where it'd matter. We actually
have a policy against using tabs at all in SGML files, for example.
regards, tom lane
On Wed, Jul 23, 2014 at 06:12:59PM -0400, Robert Haas wrote: > On Wed, Jul 23, 2014 at 4:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On Mon, Jul 21, 2014 at 10:52 AM, Emre Hasegeli <emre@hasegeli.com> wrote: > >>> The first two shapes on src/test/regress/sql/polygon.sql do not make > >>> sense to me. > > > >> Well, I think the number of tabs that makes them look best depends on > >> your tab-stop setting. At present, I find that with 8-space tabs > >> things seem to line up pretty well, whereas with your patch, 4-space > >> tabs line up well. > > > > Perhaps we should expand tabs to spaces in those ascii-art diagrams? > > > >> Personally, though, my vote would be to just leave it the way it is. > >> I don't think there's really a problem here in need of solving. > > > > I concur with doubting that changing the actual regression test cases > > is a good thing to do, at least not without more analysis. But "the > > pictures make no sense with the wrong tab setting" is a pretty simple > > issue, and one that I can see biting people. > > AFAICT, the pictures make no sense with the right tab setting, either. > The possibility that someone might use the wrong tab setting is just > icing on the cake. I extracted Emre's diagram adjustments from the patch and applied it, and no tabs now. Emre, I assume your regression changes did not affect the diagram contents. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
> I extracted Emre's diagram adjustments from the patch and applied it, > and no tabs now. Emre, I assume your regression changes did not affect > the diagram contents. Thank you for looking at it. I wanted to make the tests consistent with the diagrams. Now they look better but they still don't make sense with the tests. I looked at it some more, and come to the conclusion that removing them is better than changing the tests.
On Tue, Oct 14, 2014 at 05:40:06PM +0300, Emre Hasegeli wrote: > > I extracted Emre's diagram adjustments from the patch and applied it, > > and no tabs now. Emre, I assume your regression changes did not affect > > the diagram contents. > > Thank you for looking at it. I wanted to make the tests consistent > with the diagrams. Now they look better but they still don't make > sense with the tests. I looked at it some more, and come to the > conclusion that removing them is better than changing the tests. OK, unless I hear more feedback I will remove the diagrams. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Tue, Oct 14, 2014 at 11:00:47AM -0400, Bruce Momjian wrote: > On Tue, Oct 14, 2014 at 05:40:06PM +0300, Emre Hasegeli wrote: > > > I extracted Emre's diagram adjustments from the patch and applied it, > > > and no tabs now. Emre, I assume your regression changes did not affect > > > the diagram contents. > > > > Thank you for looking at it. I wanted to make the tests consistent > > with the diagrams. Now they look better but they still don't make > > sense with the tests. I looked at it some more, and come to the > > conclusion that removing them is better than changing the tests. > > OK, unless I hear more feedback I will remove the diagrams. Removed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +