Обсуждение: Some notes on pgAdmin
1. Going through the code, I noticed that quite a few files use 4 spaces for indent on most of their lines, but have some lines using tab characters. Usually it’s newer patches that introduced the tabs, but not always. Mixing both kinds of indents is confusing.
2. Pgadmin3.wxs should not have hard-coded paths referring to “C:\Program Files”. Some people have their program files directory on another drive, and anyone who is running 64-bit windows will find this doesn’t work, because the directory is “C:\Program Files (x86)”. Wixs has variables that point to the right one. Or use the environment variable “VCINSTALLDIR”, but I think the former is easier.
3. For the same reason, pgAdmin code that searches for client software (pgDump) shouldn’t assume “C:\Program Files”. This doesn’t work at all on 64-bit windows. The environment variables “%ProgramFiles(x86)%” or “%ProgramFiles%” points to the right directory (if they are set). “ProgramFiles(x86)” should be preferred if set, because that is where 32-bit apps like Postgres get installed. There may be a better way to do this.
4. Why do you include the VC 8 redistributable files directly, instead of using the standard merge modules: Microsoft_VC80_CRT_x86.msm and policy_8_0_Microsoft_VC80_CRT_x86.msm (usually in “C:\Program Files\Common Files\Merge Modules”)?
5. The current installer files don’t work for Visual Studio 2008, as compiles from that need the VC90 redistributables.
On Sat, Feb 28, 2009 at 12:19 AM, Chuck McDevitt <cmcdevitt@greenplum.com> wrote: > 1. Going through the code, I noticed that quite a few files use 4 > spaces for indent on most of their lines, but have some lines using tab > characters. Usually it’s newer patches that introduced the tabs, but not > always. Mixing both kinds of indents is confusing. Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm beginning to this we should use tabs instead, and see if we can't run something akin to pgindent over the code to clean it up nicely. What do people prefer - tabs or spaces? > 2. Pgadmin3.wxs should not have hard-coded paths referring to > “C:\Program Files”. Some people have their program files directory on > another drive, and anyone who is running 64-bit windows will find this > doesn’t work, because the directory is “C:\Program Files (x86)”. Wixs has > variables that point to the right one. Or use the environment variable > “VCINSTALLDIR”, but I think the former is easier. Agreed - I see that's in the Greenplum patch though, so we'll pickup the fix from that. > 3. For the same reason, pgAdmin code that searches for client software > (pgDump) shouldn’t assume “C:\Program Files”. This doesn’t work at all on > 64-bit windows. The environment variables “%ProgramFiles(x86)%” or > “%ProgramFiles%” points to the right directory (if they are set). > “ProgramFiles(x86)” should be preferred if set, because that is where 32-bit > apps like Postgres get installed. There may be a better way to do this. That's fixed in SVN - thanks for pointing out the error. > 4. Why do you include the VC 8 redistributable files directly, instead > of using the standard merge modules: Microsoft_VC80_CRT_x86.msm and > policy_8_0_Microsoft_VC80_CRT_x86.msm (usually in “C:\Program Files\Common > Files\Merge Modules”)? Hangover from the pre-VC 2005 days that noone ever bothered to fix. I'll look at it. > 5. The current installer files don’t work for Visual Studio 2008, as > compiles from that need the VC90 redistributables. I don't know that any of us have tried building with 2K8 yet anyway, but if/when point 4 is fixed, I'll be happy to accept a patch if anyone can figure out a nice way to automatically pickup the correct runtimes regardless of VC++ version. Thanks! -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Sat, Feb 28, 2009 at 12:19 AM, Chuck McDevitt > <cmcdevitt@greenplum.com> wrote: >> 1. Going through the code, I noticed that quite a few files use 4 >> spaces for indent on most of their lines, but have some lines using tab >> characters. Usually it’s newer patches that introduced the tabs, but not >> always. Mixing both kinds of indents is confusing. > > Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm > beginning to this we should use tabs instead, and see if we can't run > something akin to pgindent over the code to clean it up nicely. > > What do people prefer - tabs or spaces? > Spaces seem better to me. But I wouldn't say I didn't put some tabs in some files. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Mon, Mar 9, 2009 at 3:51 PM, Dave Page <dpage@pgadmin.org> wrote: >> 4. Why do you include the VC 8 redistributable files directly, instead >> of using the standard merge modules: Microsoft_VC80_CRT_x86.msm and >> policy_8_0_Microsoft_VC80_CRT_x86.msm (usually in “C:\Program Files\Common >> Files\Merge Modules”)? > > Hangover from the pre-VC 2005 days that noone ever bothered to fix. > I'll look at it. Change committed. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page wrote: > On Sat, Feb 28, 2009 at 12:19 AM, Chuck McDevitt > <cmcdevitt@greenplum.com> wrote: >> 1. Going through the code, I noticed that quite a few files use 4 >> spaces for indent on most of their lines, but have some lines using tab >> characters. Usually it’s newer patches that introduced the tabs, but not >> always. Mixing both kinds of indents is confusing. > > Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm > beginning to this we should use tabs instead, and see if we can't run > something akin to pgindent over the code to clean it up nicely. > > What do people prefer - tabs or spaces? There'd be a point to having the same convention as the main project. I'm not sure it's worth going over the whole code with pgindent or similar though - it makes it so much harder to backtrack the code in svn. Especially since we haven't had a standard before, it'll likely touch way too much code. But it'd still be a good thing to have a standard for new code. //Magnus
> > Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm > beginning to this we should use tabs instead, and see if we can't run > something akin to pgindent over the code to clean it up nicely. > > What do people prefer - tabs or spaces? I definitely prefer spaces. Too many tools assume tab=8 spaces. > > I don't know that any of us have tried building with 2K8 yet anyway, > but if/when point 4 is fixed, I'll be happy to accept a patch if > anyone can figure out a nice way to automatically pickup the correct > runtimes regardless of VC++ version. > I started out using VC 9 (2008), and everything works fine except you need to include the merge modules from VC 9 for theruntime (and it has to be the merge modules, including the files by themselves doesn't work right). I went back to VC 8 to keep compatible with what others are using for PgAdmin.
On Mon, Mar 9, 2009 at 2:06 PM, Chuck McDevitt <cmcdevitt@greenplum.com> wrote:
>I definitely prefer spaces. Too many tools assume tab=8 spaces.
> Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm
> beginning to this we should use tabs instead, and see if we can't run
> something akin to pgindent over the code to clean it up nicely.
>
> What do people prefer - tabs or spaces?
I started out using VC 9 (2008), and everything works fine except you need to include the merge modules from VC 9 for the runtime (and it has to be the merge modules, including the files by themselves doesn't work right).
>
> I don't know that any of us have tried building with 2K8 yet anyway,
> but if/when point 4 is fixed, I'll be happy to accept a patch if
> anyone can figure out a nice way to automatically pickup the correct
> runtimes regardless of VC++ version.
>
I went back to VC 8 to keep compatible with what others are using for PgAdmin.
Hi, I know that last year I tried to capture coding standard for new code, then I create a wiki at postgresql, probably is not 100% accurate but it can be used as a start for the creation of the project standard for codification.
url: http://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style
Here too, it's the location to explain the process (and problems and solutions) of compiling pgAdmin with different versions of VC++ or other operative systems and/or compilers.
Regards, Luis.
Em Seg, 2009-03-09 às 15:51 +0000, Dave Page escreveu: > Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm > beginning to this we should use tabs instead, and see if we can't run > something akin to pgindent over the code to clean it up nicely. > > What do people prefer - tabs or spaces? 4 spaces = 4 bytes 1 tab = 1 byte For me this is the difference, file size. ":) -- Dickson S. Guedes mail/xmpp: guedes@guedesoft.net - skype: guediz http://guedesoft.net - http://planeta.postgresql.org.br
Magnus Hagander escreveu: > I'm not sure it's worth going over the whole code with pgindent or > similar though - it makes it so much harder to backtrack the code in > svn. Especially since we haven't had a standard before, it'll likely > touch way too much code. > What about do it after next release? Looking at the source code, almost everything uses 4 spaces per level so we could go through this way. -- Euler Taveira de Oliveira http://www.timbira.com/
On Tue, Mar 10, 2009 at 12:38 AM, Euler Taveira de Oliveira <euler@timbira.com> wrote: > Magnus Hagander escreveu: >> I'm not sure it's worth going over the whole code with pgindent or >> similar though - it makes it so much harder to backtrack the code in >> svn. Especially since we haven't had a standard before, it'll likely >> touch way too much code. 4 spaces has been the unofficial standard for years, and I a) bleat if I see a patch with tabs (I think I did to Chuck actually) and b) fix them if I spot them (usually if I'm editing in vim). > What about do it after next release? Looking at the source code, almost > everything uses 4 spaces per level so we could go through this way. I'm not convinced it would make so much difference that we'd have trouble tracing back SVN history - I'd probably start with pgagent anyway and see how that went. What I'm less convinced about is that pgindent will know how to format C++ properly, though I'm sure there will be other tools that could do the job. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page wrote: > On Tue, Mar 10, 2009 at 12:38 AM, Euler Taveira de Oliveira > <euler@timbira.com> wrote: >> Magnus Hagander escreveu: >>> I'm not sure it's worth going over the whole code with pgindent or >>> similar though - it makes it so much harder to backtrack the code in >>> svn. Especially since we haven't had a standard before, it'll likely >>> touch way too much code. > > 4 spaces has been the unofficial standard for years, and I a) bleat if > I see a patch with tabs (I think I did to Chuck actually) and b) fix > them if I spot them (usually if I'm editing in vim). Ok. >> What about do it after next release? Looking at the source code, almost >> everything uses 4 spaces per level so we could go through this way. > > I'm not convinced it would make so much difference that we'd have > trouble tracing back SVN history - I'd probably start with pgagent > anyway and see how that went. The point is, you kill the ability to do "svn blame". And doing a diff with a revision beyond when you did the indent run, will show lots of irrelevant stuff. You can ignore whitespace for tihs part (not the one above afaik), but not things like brace-changes. > What I'm less convinced about is that pgindent will know how to format > C++ properly, though I'm sure there will be other tools that could do > the job. yeah, I doubt it'll do it right - I read you as "a tool similar to it". But if we want to do it, it's worth seeing if there's a switch somewhere for pgindent (which just runs indent underneath iirc) //Magnus
Em Seg, 2009-03-09 às 21:38 -0300, Euler Taveira de Oliveira escreveu:
> Magnus Hagander escreveu:
> > I'm not sure it's worth going over the whole code with pgindent or
> > similar though - it makes it so much harder to backtrack the code in
> > svn. Especially since we haven't had a standard before, it'll likely
> > touch way too much code.
> >
> What about do it after next release? Looking at the source code, almost
> everything uses 4 spaces per level so we could go through this way.
I don't know if somebody did it yet bu just for information.
$ cd ~/src/pgadmin3/pgadmin
$ LC_ALL=en_US svn update
At revision 7657.
$ find ./ -name "*.cpp" -exec egrep "^([[:space:]]{4}|\t)+" {} \; | wc
-l
47911 matching lines started with 4 spaces or tabs
$ find ./ -name "*.cpp" -exec egrep -H "^([[:space:]]{4}|\t)+" {} \; |
cut -d':' -f1 | sort | uniq | wc -l
273 files matching lines started with 4 space or tabs
[]s
Dickson S. Guedes
mail/xmpp: guedes@guedesoft.net - skype: guediz
http://guedesoft.net - http://planeta.postgresql.org.br
Em Ter, 2009-03-10 às 10:37 -0300, Dickson S. Guedes escreveu:
> I don't know if somebody did it yet bu just for information.
>
> $ cd ~/src/pgadmin3/pgadmin
> $ LC_ALL=en_US svn update
> At revision 7657.
>
> $ find ./ -name "*.cpp" -exec egrep "^([[:space:]]{4}|\t)+" {} \; | wc
> -l
> 47911 matching lines started with 4 spaces or tabs
>
> $ find ./ -name "*.cpp" -exec egrep -H "^([[:space:]]{4}|\t)+" {} \; |
> cut -d':' -f1 | sort | uniq | wc -l
> 273 files matching lines started with 4 space or tabs
I forgot two more...
find ./ -name "*.cpp" -exec egrep -H "^(\t)+" {} \; | cut -d':' -f1 |
sort | uniq | wc -l
31 files matching lines started with tabs...
find ./ -name "*.cpp" -exec egrep -H "^[[:space:]]+\t+" {} \; | cut
-d':' -f1 | sort | uniq | wc -l
182 files matching lines started wit spaces followed by tabs..
[]s
Dickson S. Guedes
mail/xmpp: guedes@guedesoft.net - skype: guediz
http://guedesoft.net - http://planeta.postgresql.org.br
I don't think we want to go the pgindent route, but a simple script that takes all files, converts tabs to 4 spaces, andchecks back in any changed files, would be a good idea.
On Tue, Mar 10, 2009 at 8:00 PM, Chuck McDevitt <cmcdevitt@greenplum.com> wrote: > I don't think we want to go the pgindent route, but a simple script that takes all files, converts tabs to 4 spaces, andchecks back in any changed files, would be a good idea. > Yeah, that's more or less the conclusion I came to. Anyone got the energy to whip up an appropriate shell script? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com