Github user afs commented on the pull request:
https://github.com/apache/jena/pull/56#issuecomment-99023698
This PR is quite big so comments will be incremental. In fact, it might be
better to tease out different sets of changes. Could you give a quick overview
of the changes?
Overall, this looks like a great set of changes, including using classes
available in Google Guava to replace ones in Jena (e.g. MultiMap, MultiSet).
#### `oaj.atlas.lib.Closeable`
Adding `extends java.io.Closeable` causes about 76 more warnings in the
code base because `java.io.Closeable` is interpreted by the compiler specially.
Some of the classes that use `oaj.atlas.lib.Closeable` aren't suitable for
try-resource.
Maybe some of these classes don't really need closeable but that needs
checking. Can we pull this out as a separate strand and not include it in the
general Java8 overhaul?
#### `NodeTransform`
This is an important abstraction in the SPARQL optimizer. Can we keep it,
define it as `extends Function<Node, Node>` and keep use of `NodeTransform`?
(There are times in Java when it would be nice to say "interface/class A is the
same as B" where B is a complex expression, but you can't). It makes finding
`NodeTransform`s easier and makes interfaces clearer as to purpose. The
optimizer code is not general java code.
#### `@Deprecated`
I noticed this on `Lib.equal` and it seems to be a style in the PR to say
"Prefer" in the javadoc. It gets highlighted in javadoc if `@deprecated` is
used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
