gump-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leo Simons <m...@leosimons.com>
Subject Re: [jira] Updated: (GUMP-149) allow gump to bootstrap maven
Date Thu, 18 Aug 2005 19:21:44 GMT
whoohooh! A patch! Looks pretty sweet. Before we chuck it in, some
comments...

First of, I presume you have some tests (eg project.xml files and a
sample workspace) against which you run this code. Could you submit
those too?

> +    def __init__(self, project, target, buildfile="maven.xml",
basedir=None, properties=None):

Isn't using 'maven.xml' mandatory? I think you can set the POM but not
the buildfile...

> +	#command line
> +	args = ["maven jar "]

If I'm not mistaken args should be one-entry per array, eg

 ["maven", "jar"]

also, the "target" defined in the model as above is probably the maven
"goal" to use instead of the hardcoded "jar"?

Leo Simons wrote:
> Index: pygump/python/gump/engine/map.py

could map.py go into gump/util or gump/plugins/java somewhere? Also,
it'd be cool if this could be a config file of some sort that is read
in, perhaps a format as simple as

avalon-framework avalon-framework-api
axis ws-axis

Note that the 'gump' plugin for maven has many more of these mappings
that would be nice to steal.

> Index: pygump/python/gump/engine/mavenizer.py
> ===================================================================
> +	if maven_node:
> +		#parse DOM creating new gumped-maven file
> +		#then return file location to be used instead
> +		#of original maven file
> +		gumped_maven_href = _parse_maven_file( maven_node, download_func,
> get_vfs )
> +		return gumped_maven_href
> +	else:
> +		return 'broken'

you want to 'raise' an exception here.

> +def _parse_maven_file( project, download_func, get_vfs ):

this looks pretty comprehensive!

> +	mavenfile = open("%s\\%s" % (home_dir, filename), "w")
                            ^^^ use os.path.seperator (or whatever its
                                called)

however, using the StringIO module would probably avoid the need for
using a file at all, which would rock even more.

> Index: pygump/python/gump/config.py
> ===================================================================
> --- pygump/python/gump/config.py	(revision 233131)
> +++ pygump/python/gump/config.py	(working copy)
> @@ -105,6 +105,7 @@
>          log.info("Not running updates! (pass --do-updates to enable
them)")
>
>      if config.do_build:
> +	from gump.model import Maven
>          from gump.model import Ant
>          from gump.model import Script

looks like a TAB character in there. Seems there's several more in your
patch. Please get rid of those and just use spaces :)

> Index: pygump/python/gump/util/executor.py
> ===================================================================
> --- pygump/python/gump/util/executor.py	(revision 233131)
> +++ pygump/python/gump/util/executor.py	(working copy)
> @@ -38,7 +38,7 @@
>  __copyright__ = "Copyright (c) 2004-2005 The Apache Software Foundation"
>  __license__   = "http://www.apache.org/licenses/LICENSE-2.0"
>
> -import sys
> +import sys, os

huh, what? Why? Seems there's a few "loose ends" in the patch...

> Index: pygump/python/gump/util/io.py
> ===================================================================
> --- pygump/python/gump/util/io.py	(revision 233131)
> +++ pygump/python/gump/util/io.py	(working copy)
> @@ -99,6 +99,9 @@
>              self.cachedir = os.path.normpath(os.path.abspath(cachedir))
>          else:
>              self.cachedir = None
> +
> +    def return_path(self):
> +	return self.filesystem_root

feel free to just use the property directly, eg replace calls to
myvfs.return_path() with simply

  myvfs.filesystem_root

instead of writing "accessors". Sun did the world a lot of harm by
inventing getters/setters for java. Python certainly doesn't need them.


cheers,


Leo


---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@gump.apache.org
For additional commands, e-mail: general-help@gump.apache.org


Mime
View raw message