[modules-dev] Initial repository management tool

[Dave Bristor]

Stanley M. Ho wrote:
> Hi Dave,
> 
> Looks like a good start. Several comments inline:
> 
> - src/share/classes/sun/module/tools/ToolBase.java
> 
> 35 public class ToolBase {
> 
> Should be package private.

Fixed.

> 
> - src/share/classes/sun/module/tools/JRepo.java
> 
> Remove empty line at #43 and #46.

Fixed.

> 
>   76             usage = "Usage: jrepo <cmd>, where cmd is follows one 
> of the patterns below";
> 
> Perhaps "command" instead of "cmd".

Fixed.

> 
>  68     private static final Map<String, Command> commands = new 
> LinkedHashMap<String, Command>();
>   75         if (usage == null) {
>   76             usage = "Usage: jrepo <cmd>, where cmd is follows one 
> of the patterns below";
>   77             new ListCommand();
>   78             new FindCommand();
>   79             new QueryCommand();
>   80         }
>  167             commands.put(name, this);
>  168             String u = usage();
>  169             if (u != null) {
>  170                 usage += "\n\t" + usage();
>  171             }
>  330         for (Map.Entry<String, Command> entry : commands.entrySet()) {
>  331             if (entry.getKey().startsWith(args[0])) {
>  332                 cmd = entry.getValue();
>  333                 break;
>  334             }
>  335         }
>  416     void usageError() {
>  417         error(usage);
>  418     }
> 
> I don't like the idea of having the Command constructor to bring side 
> effect to "usage" and "commands" in the outer class. I think removing 
> these side effects could simply the code and make it more readable, e.g.
> 
> private state final List<Command> commands = new ArrayList<Command>();
> ...
>     commands.add(new ListCommand());
>     commands.add(new FindCommand());
>     commands.add(new QueryCommand());
> ...
> for (Command command : commands) {
>    if (command.getName().startsWith(args[0])) {
>         cmd = entry.getValue();
>         break;
>    }
> }
> ...
> private void usageError() {
>    StringBuilder builder = new StringBuilder();
>    builder.append("Usage: jrepo <command>, where command follows one of 
> the patterns below\n");
>    for (Command command : commands) {
>        builder.append(command.getUsage());
>         builder.append("\n");
>    }
>    error(builder.toString());
> }

I refactored it all slightly differently, but definitely removed the 
side-effect-ness.

> 
>  165     abstract class Command {
>  223     class ListCommand extends Command {
>  261     class FindCommand extends Command {
>  303     class QueryCommand extends FindCommand {
> 
> These classes should be private static if possible.

private: fixed.

static: harder: They're making use of usageError, which is inherited from 
Toolbase...the output methods in ToolBase depend on being initialized with 
streams during construction.  So for now, just private abstract.

>  103         if (repo == null) {
>  104             err.println("can't access repository " + 
> repositoryLocation);
>  105             return false;
>  106         }
> 
> Is this check really necessary? I would have assumed the method has 
> already returned if there was exception in #95-#100.

Fixed.

> I'm surprised we have to configure the metadata directory in the 
> repository in #139-#145. I think the URLRepository implementation should 
> do this automatically by default if no metadata directory is specified.

If not configured, it will do so automatically.  But currently, the 
automatically determined directory is always the same one, and so might result 
in inadvertent sharing or other bizarre behavior.  Probably, the default 
directories for the repository classes should be based on temporary names.  If 
that seems right to you, I'll file a bug for it and remove the configuration 
in JRepo.

> 
>  246                 List<ModuleArchiveInfo> installed = repo.list();
>  247                 for (ModuleArchiveInfo mai : installed)
> 
> Should combine into
> 
>     for (ModuleArchiveInfo mai : repo.list())

Fixed.

> 
>  308         String usage() { return null; }
> 
> Should either not override the method, or return usage string specific 
> for "query".

Not really: by explicitly returning null, find and query have the same usage 
String, provided by find.

> The getRepositoryText method looks like something Repository.toString() 
> should provide. If not, at least we could clean it up as:
> 
>  private String getRepositoryText(Repository repo) {
>      URL u = repo.getSourceLocation();
>         if (u == null) {
>             return "Bootstrap repository";
>         } else {
>         return "Repository " + u.toExternalForm();
>     }
>  }

I prefer to avoid multiple returns, hence the current approach.  Not a big 
deal here; I can change it if it's important.  Also, I propose to move this 
method as you suggest, but want to keep that separate fromt he current 
examination of JRepo.

One more thing, I renamed "Curser" to "RepositoryVisitor", and changed the 
method name recurse -> visit.  I just couldn't think of the right name at first!

Do you have any suggestions for writing tests?  I've written a very basic one 
for now.  Perhaps in the fullness of time, SQE can do a much better job.

Webrev updated, includes test

http://javaweb.sfbay/java/jdk/ws/libs/rev/6559117/


Thanks,
	Dave

> 
> That's it for now.
> 
> - Stanley
>