Re: Information for added functionalities - Mailing list pgadmin-hackers

From Dave Page
Subject Re: Information for added functionalities
Date
Msg-id CA+OCxozRmtxRbH_A4HcegTwcE+Chgd6JGSHhA2eArpGXtn1oGA@mail.gmail.com
Whole thread Raw
In response to Re: Information for added functionalities  (Vladimir Kokovic <vladimir.kokovic@gmail.com>)
Responses Re: Information for added functionalities
List pgadmin-hackers
Hi

As I mentioned below, there are issues with the patches that need to
be cleaned up before they can be removed. A sizeable percentage of
patch3 for example changes things that are completely unrelated to
what the patch claims to do, or just changes paths for unrelated files
in the makefiles.

Please clean that up, and then post the patches *separate* email
threads. Noone can review them as they are now.

Thanks.

On Wed, Sep 19, 2012 at 3:46 AM, Vladimir Kokovic
<vladimir.kokovic@gmail.com> wrote:
> Hi,
>
> Separated in three patches:
>  patch1.tar.gz - pg_scanner and frmQuery.cpp with multiple result sets support
>  patch2.tar.gz - sysSettings::Flush
>  patch3.tar.gz - copy/paste table(s) with flush server settings to disk
>
> Best regards
> Vladimir Kokovic
> Belgrade, Serbia, 19.September 2012
>
>
> On 9/18/12, Dave Page <dpage@pgadmin.org> wrote:
>> Hi
>>
>> On Sat, Sep 15, 2012 at 6:53 AM, Vladimir Kokovic
>> <vladimir.kokovic@gmail.com> wrote:
>>> Hi,
>>>
>>> Separated in three patches:
>>> patch1.tar.gz - pg_scanner and frmQuery.cpp with multiple result sets
>>> support
>>> patch2.tar.gz - sysSettings::Flush
>>> patch3.tar.gz - copy/paste table(s)
>>
>> Please post the patches publicly, i.e. to the mailing list, on
>> individual threads.
>>
>> I took a very brief look at 2 and 3, and noticed some issues you'll
>> want to cleanup first:
>>
>> - There are whitespace-only changes which should be removed.
>>
>> - There are completely unrelated changes to some files - e.g. patch 3
>> removes "$(srcdir)/" from the file paths in pgadmin/ui/module.mk, and
>> there are changes to function argument handling.
>>
>> - There are still some parts of one new feature in the other - e.g.,
>> patch 3 seems to contain code to flush server settings to disk.
>>
>> - I see very few comments explaining what code is supposed to do. This
>> is not necessary if code is self explanatory, but is needed in more
>> complex code blocks.
>>
>> I'm assuming that patch 1 has similar issues. Fixing them first will
>> make the patches *much* smaller and will make it possible to review
>> them.
>>
>> Thanks.
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


pgadmin-hackers by date:

Previous
From: Vladimir Kokovic
Date:
Subject: Re: Information for added functionalities
Next
From: Vladimir Koković
Date:
Subject: Re: Information for added functionalities