Prev: Need a Promotion for a good life. alamode adenia absonant
Next: [HACKERS] PostGIS vs. PGXS in 9.0beta3
From: Andres Freund on 30 Jul 2010 12:33
On Friday 30 July 2010 18:21:49 Greg Smith wrote:
> - break;
> + break;
>
> This happened because you added two invisible tabs to the end of this
> line. This makes the patch larger for no good reason and tends to
If you want to remove changes like this using:
"git checkout -p HEAD"
is very useful if youre using git. It allows you to selectively revert hunks
of not-checked-in changes...

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 30 Jul 2010 12:31
On Fri, Jul 30, 2010 at 12:21 PM, Greg Smith <greg(a)2ndquadrant.com> wrote:
>�If you had those done for this patch, I
> wouldn't have to ask for code examples; I could just look at the source to
> the regression output and see how to use it against the standard database
> the regression samples create, and then execute against.

I agree. While not every feature needs regression tests, something
this complex certainly does. Also, running the regression tests
(frequently!) can help you realize when you've broken the existing
code before too much time goes by and it's no longer easy to figure
out which change is responsible for the breakage.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Boxuan Zhai on 1 Aug 2010 01:03
2010/7/31 Greg Smith <greg(a)2ndquadrant.com>

> Boxuan Zhai wrote:
>
>> I create a clean patch file (no debug clauses). See the attachment.
>>
>
> Some general coding feedback for you on this.
>
> Thanks for your consideration!


> It's not obvious to people who might want to try this out what exactly they
> are supposed to do. Particularly for complicated patches like this, where
> only a subset of the final feature might actually be implemented, some sort
> of reviewer guide suggesting what should and shouldn't work would be
> extremely helpful. I recall there was some sort of patch design guide in an
> earlier version of this patch; it doesn't seem to be there anymore. Don't
> remember if that had any examples in it.
>
> I am now working on fixing a bug which makes the system unable to initdb. I
will update my page in postgres Wiki with a detailed instruction of my
implementation and testing examples soon, with my next patch file.


> Ultimately, the examples of working behavior for this patch will need to be
> put into code. The way that happens is by adding working examples into the
> regression tests for the database. If you had those done for this patch, I
> wouldn't have to ask for code examples; I could just look at the source to
> the regression output and see how to use it against the standard database
> the regression samples create, and then execute against. This at least lets
> you avoid having to generate some test data, because there will already be
> some in the regression database for you to use. There is an intro this
> topic at http://wiki.postgresql.org/wiki/Regression_test_authoring Another common way to generate test data is to run pgbench which creates 4
> tables and populates them with data.
>
> I will try to add my testing examples to the gregression folder.


> As far as the patch itself goes, you have some work left on cleaning that
> up still you'll need to do eventually. What I would suggest is actually
> reading the patch itself; don't just generate it and send it, read through
> the whole thing like someone new to it would do. One way you can improve
> what you've done already is to find places where you have introduced changes
> to the code structure just through editing. Here's an example of what I'm
> talking about, from line 499 of your patch:
>
> - break;
> + break;
> This happened because you added two invisible tabs to the end of this line.
> This makes the patch larger for no good reason and tends to infuriate
> people who work on the code. There's quite a bit of extra lines added in
> here too that should go. You should consider reducing the ultimate size of
> the patch in terms of lines a worthwhile use of your time, even if it
> doesn't change how things work. There's lots of examples in this one where
> you put two or three lines between two statements when a single one would
> match the look of the code in that file. A round of reading the diff
> looking for that sort of problem would be useful.
>
> Another thing you should do is limit how long each line is when practical.
> You have lots of seriously wide comment lines here right now in particular.
> While there are some longer lines in the PostgreSQL code right now, that's
> code, not comments. And when you have a long line and a long comment, don't
> tack the comment onto the end. Put it on the line above instead. Also,
> don't use "//" in the middle of comments the way you've done in a few places
> here.
>
> Sorry for these mistakes, again. I promise that the same thing will not
happen in my next patch.



> Getting some examples sorted out and starting on regression tests is more
> important than the coding style parts I think, just wanted to pass those
> along while I noticed them reading the patch, so you could start looking out
> for them more as you continue to work on it.
>
> --
> Greg Smith 2ndQuadrant US Baltimore, MD
> PostgreSQL Training, Services and Support
> greg(a)2ndQuadrant.com www.2ndQuadrant.us <http://www.2ndquadrant.us/>
>
>
From: Josh Berkus on 3 Aug 2010 18:14

> And, I have tested the running of MERGE command with different
> situations. I am sorry that I didn't create regression test files,
> because I am not sure how to add new files in the git package. But, I
> have written web pages in Postgres Wiki. I explain the details of my
> implementation and a set of testing examples.

Can someone help Boxuan with how to write regression tests?

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Josh Berkus on 3 Aug 2010 18:23

> https://wiki.postgresql.org/wiki/Implementation_detalis
> https://wiki.postgresql.org/wiki/Test_examples

These pages were confusingly named, so I just moved them:

https://wiki.postgresql.org/wiki/MergeTestExamples
https://wiki.postgresql.org/wiki/MergeImplementationDetails

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: Need a Promotion for a good life. alamode adenia absonant
Next: [HACKERS] PostGIS vs. PGXS in 9.0beta3