A lice-sized code review (edward localrev 85)
edy...@gmail.com
I'd like you to do a code review. Please review the following patch:
----------------------------------------------------------------------
r85: edward | 2007-09-14 16:36:51 +0800
Created a simple ant file to make the later work easier.
----------------------------------------------------------------------
=== build.properties.sample
==================================================================
--- build.properties.sample (revision 84)
+++ build.properties.sample (revision 85)
@@ -0,0 +1,2 @@
+ECLIPSE_HOME = /usr/lib/eclipse
+VERSION = 0.2.5
=== build.xml
==================================================================
--- build.xml (revision 84)
+++ build.xml (revision 85)
@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project basedir="." default="build" name="EEEdit">
+ <property file="build.properties"/>
+ <path id="eeedit.classpath">
+ <fileset dir="${ECLIPSE_HOME}/plugins" includes="org.eclipse.*.jar"/>
+ </path>
+ <target name="init">
+ <copy includeemptydirs="false" todir="org.eeedit.vimclient_${VERSION}">
+ <fileset dir="eeedit"
+ excludes="**/*.launch, **/*.java, **/*.properties"/>
+ </copy>
+ </target>
+ <target name="clean">
+ <delete>
+ <fileset dir="org.eeedit.vimclient_${VERSION}" includes="**/*.class"/>
+ </delete>
+ </target>
+ <target name="cleanall">
+ <delete dir="org.eeedit.vimclient_${VERSION}" />
+ </target>
+ <target depends="build-subprojects,build-project" name="build"/>
+ <target name="build-subprojects"/>
+ <target depends="init" name="build-project">
+ <javac destdir="org.eeedit.vimclient_${VERSION}">
+ <src path="eeedit/src" />
+ <classpath refid="eeedit.classpath" />
+ </javac>
+ </target>
+</project>
This is a semiautomated message from "svkmail". Complaints or suggestions?
Mail edy...@gmail.com.
David Terei
Thanks for the patch.
What is it you are trying to achieve here though? An ant script for
compilation? Or is it really meant for packaging, making it easy for us to
provide new releases? I haven't added them to the repository but Eclipse can
generate ant scripts for the latter task.
Few questions/issues:
1. Where is this script meant to be stored? Seems as if it should be stored
above the trunk directory. So
- tmp
| - build.xml
| - eeedit/
| | - src/
| | - plugin.xml
| | ...
And I don't really like this since then in SVN it will be stored outside the
trunk folder.
2. When copying the folder, you exclude .properties files. This shouldn't be
(I assume you just want to exclude build.properties anyway), since
.properties files are used for localization. (We have one file for this
already although it is little more than a stub really).
3. A little documentation would be nice.
Best regards,
David Terei
Edward L. Fox
On 9/15/07, David Terei <david...@gmail.com> wrote:
> Hi Edward,
>
> Thanks for the patch.
With pleasure.
But in fact it's a "code review request". In another open source
project I am participating, every member is required to send out a
"code reviewer request" before doing the real submitting. Everyone has
to obtain at least one "positive comment" from another developer or he
will not be allowed to submit his change list.
I highly recommended that we also follow this principle during our
development. What's your opinion?
>
> What is it you are trying to achieve here though? An ant script for
> compilation? Or is it really meant for packaging, making it easy for us to
> provide new releases? I haven't added them to the repository but Eclipse can
> generate ant scripts for the latter task.
I just mean to have a script for compiling the project. If you want,
packaging feature can also be added soon. A handy command prompt based
tool chain is quite important to me.
As I'm already granted the writing access to the SVN repository, you
don't need to add my changes to the repository for me. I'll emit
another code review request according to your requests. If you think
it's acceptable, just reply "Looks good to me" and I'll submit it
myself.
> Few questions/issues:
>
> 1. Where is this script meant to be stored? Seems as if it should be stored
> above the trunk directory. So
>
> - tmp
~~~ Here is the top-level of the trunk.
> | - build.xml
~~~~~~~~~ Yes, it's the right position.
> | - eeedit/
~~~~~~~ But it's not the top-level of the trunk.
> | | - src/
> | | - plugin.xml
> | | ...
>
> And I don't really like this since then in SVN it will be stored outside the
> trunk folder.
All my change lists are based at the top-level of the "trunk" folder.
I think this information will make the code reviewing more easily
later on. Sorry for not mentioning this in my previous posts.
> 2. When copying the folder, you exclude .properties files. This shouldn't be
> (I assume you just want to exclude build.properties anyway), since
> .properties files are used for localization. (We have one file for this
> already although it is little more than a stub really).
I see. That's my fault. I'll change it.
> 3. A little documentation would be nice.
I'll document the whole project later on. It's my next first-priority
task. I'll need a UML diagram to make myself easier to understand the
project. It is still a very small project so I think it will be quite
easy for me to join in.
> Best regards,
> David Terei
>
> [...]
Best regards,
Edward L. Fox
David Terei
On 9/16/07, Edward L. Fox <edy...@gmail.com> wrote:
>
> Hi David,
>
> On 9/15/07, David Terei <david...@gmail.com> wrote:
> > Hi Edward,
> >
> > Thanks for the patch.
>
> With pleasure.
>
> But in fact it's a "code review request". In another open source
> project I am participating, every member is required to send out a
> "code reviewer request" before doing the real submitting. Everyone has
> to obtain at least one "positive comment" from another developer or he
> will not be allowed to submit his change list.
>
> I highly recommended that we also follow this principle during our
> development. What's your opinion?
Yeah I quite like that idea, I'll try to begin doing this as well.
I'll also try to get some developer documentation out there so that we
can start recording these policies.
Ok yep all makes sense now. I was mistakenly thinking of 'eeedit' as
being the "trunk" folder at the time I wrote the email.
>
> > 2. When copying the folder, you exclude .properties files. This
> shouldn't be
> > (I assume you just want to exclude build.properties anyway), since
> > .properties files are used for localization. (We have one file for
> this
> > already although it is little more than a stub really).
>
> I see. That's my fault. I'll change it.
>
> > 3. A little documentation would be nice.
>
> I'll document the whole project later on. It's my next first-priority
> task. I'll need a UML diagram to make myself easier to understand the
> project. It is still a very small project so I think it will be quite
> easy for me to join in.
Sounds great.