[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