Substitution (aka aliases)
mandy.chung at oracle.com
Thu Apr 12 13:28:31 PDT 2012
On 4/12/2012 11:47 AM, Alan Bateman wrote:
> I went through all the changes, good work! It's nice to get rid of the
> directories for the views along with the copy of the module info classes.
Thanks for the review, Alan.
> As you pointed out, most of the changes are in SimpleLibrary so I
> spent the most time there.
> On the lock file, it only need only be opened for writing to get an
> exclusive lock.
> What would you think of Dictionary instead of ModuleDirectory for the
> inner case name (Catalog comes to mind too but that's already used)?
> Could it extend MetaData too?
As for class name, I also thought about "Dictionary" while I think it's
more like a directory of looking up names. I can change it if you think
Dictionary is a better fit.
I also like to make ModuleDirectory and also RepoList inner classes to
extend MetaData but it requires some cleanup that I leave it for later
to do. The current MetaData class writes to the file directly but what
we want here to write to a temporary file first and then atomically
replace the file so that readers of ModuleDictionary doesn't need to
hold the exclusive lock. I keep this as one of the cleanup tasks to do.
> Minor comment but the private modifier on every method is a bit of a
> distraction in ModuleDirectory.
Agree - I'll clean them up.
> ModuleDirectory.refresh could use DirectoryStream to iterate over the
> directory and would avoid the canRead check (which isn't reliable on
> all platforms anyway). The newDirectoryStream method will fail if the
> directory can't be opened which is what is needed.
I'll take a look at that.
> A couple of minor nits, inconsistent indent in Catalog.java L143,
> RemoteRepository.java L337, Commands.java L182. One other nit is the
> copyright date on hello-alias.sh.
Will fix them. Thanks for catching them.
> Otherwise I think this is good. As you noted, the main open question
> here is whether aliases should have version numbers.
More information about the jigsaw-dev