Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Nice to see that all the tests are passing. Thanks for doing that.
I've given this a quick review and have a few top-level comments:
* Note that our code style is to explicitly use `final` for all variable
declarations that require it - i think we make exceptions on exceptions in
catch blocks and the var declaration of a `for()` loop, but that's about it. I
think you've missed a few of those.
* Could you please rename `BasicGraphManager` to `DefaultGraphManager`? We
only use the term "Basic" in one other class naming, so it's a bit of an
anomaly compared to "Default".
* Does this change introduce any compile-time breaking changes for anyone
who has depended on the `GraphManager` class (or other things you've
modified)? I'm wondering if this is a change better suited for the master
branch and 3.3.0. If it stays on `tp32` I may have to ask you to provide a
second pull request to master that can also be evaluated as part of a second
review/vote.
* The addition of `addGraph()` is nice.
* Can you elaborate on the usage of `openGraph()` - I think I get it, but
it's not clear to me based on the javadoc/default implementation
* Please add a entry (or more) to CHANGELOG for these updates.
* Please update the reference documentation to include your new Gremlin
Server configuration option here:
http://tinkerpop.apache.org/docs/current/reference/#_configuring_2 - not sure
how much you need to write here. Users of `GraphManager` have likely dug pretty
deep into the code and will see how it works. Perhaps include a link to the
javadoc for `GraphManager` and beef up the class javadoc there so that
implementers know what to do.
Just a bit of warning - from my perspective, this is a fairly involved pull
request you've started (and thanks!) so it will likely take a some time and
iterations to get through before we can get it merged.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
