[modules-dev] Initial repository management tool
Andreas Sterbenz
Andreas.Sterbenz at Sun.COM
Fri Jul 13 15:16:32 PDT 2007
Dave Bristor wrote:
>
> Webrev updated, includes test
>
> http://javaweb.sfbay/java/jdk/ws/libs/rev/6559117/
Looks fine. Some minor comments:
. ToolBase should be declared abstract for clarity
JRepo:
. DEBUG is true in the webrev
. the Boolean.getBoolean() code in the comments can fail if there is a
security manager
. commands and JRepo constructor: it would seem clearer to me if the
code to initialize the static 'commands' Map was in a static initializer
rather than under a check in the constructor
. it seems a little confusing for the Command constructor to have a side
effect of registering the class. Maybe there should be a static register
method somewhere that takes a Command object?
. getRepository(): wouldn't the fix for the XXX be to do the "new
URL()", and if that succeeds, construct a URLRepository; if it throws an
Exception, use a LocalRepository? If those constructors fail, propagate
the exceptions.
. getMAIText(): should this really say "platform?" if platform is null,
etc? Rather than an empty string, or "generic", or "n/a"?
. the distinction between "list" and "find" (and "query") seems rather
subtle. Do we need those as separate commands? Or could module name and
version merely be optional parameters?
. I assume you are updating make/sun/Makefile to build make/sun/jrepo?
Andreas.
More information about the modules-dev
mailing list