With what kind of namespaces do you expect people to use this stateful class?

@ajs6f, see: https://github.com/fcrepo4-labs/fcrepo-audit/pull/3/files#diff-3ff409abe3384fddfe4bc7d36c6ce055R261

Thank you, @awoods, but that explains very little to me. Is there some general theory of operation for this class? Knowing the quality of @escowles's work, I'm not worried-- but I would like to understand why he considered it better to alter this class than to create a new IdentifierConverter impl.

@ajs6f I started out doing just that -- and then realized that I only needed to change the hard-coded REPOSITORY_NAMESPACE variable to the baseURL, and I could use the DefaultIdentifierConverter.

@ajs6f, @escowles, I am inclined to merge this PR unless there are concerns.

I'd rather see this diverge into an AuditIdentiferConverter or something of the sort.

What is the default value for the namespace at play?

In the context of Audit, the "default" value is dynamic: baseURL + "/", as you can see here:
https://github.com/fcrepo4-labs/fcrepo-audit/pull/3/files#diff-3ff409abe3384fddfe4bc7d36c6ce055R261

It is in no way remotely clear to me how that line of code documents the use of the class under comment. At the very least, please let's include some verbal explanation of the new constructor and its parameters. "Default namespace to use for node URIs" and "Construct the graph with the provided resource namespace." doesn't begin to explain the matter. I'm confident that the thought behind this change is valuable, but I really don't understand yet why this is presented as a change to this class and not the basis of a new class.

Thanks for the perspective, @ajs6f. I will hold off until tomorrow... hoping @escowles nails it in the morning.

I will be available tomorrow morning via IRC or Hangout or what-have-ye. I don't want to hold up the functionality that @escowles is moving forward. But I do have the unpleasant feeling that we aren't all on the same page with respect to the construction of identifiers, and that's such a crucial element of repository work that I'm willing to be annoying about this.

@ajs6f My initial approach was to create a new implementation in the audit package called AuditIdentifierConverter. I looked at the DefaultIdentifierTranslator class and quickly realized I could just extend it -- if the DEFAULT_NAMESPACE variable wasn't final. So I made that change, and started looking to see what I would need to customize, and wound up with only a constructor. So I decided it was a little silly to extend a class just to add a new constructor signature, when I was already having to change the original class. I think just adding the constructor to DefaultIdentifierTranslator is the simplest way forward, and will make it more usable from any other non-web contexts.

Though I'm willing to be convince that I should be using HttpResourceConvertor instead:

https://github.com/fcrepo4-labs/fcrepo-audit/pull/3/files#diff-3ff409abe3384fddfe4bc7d36c6ce055R261

The task is to update the properties on a newly-created audit event node. Creating the triples is the easy part, but I wasn't able to get an HttpResourceConvertor instance to successfully consider the resource's URI to be in-domain. I tracked that as far as:

https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-http-commons/src/main/java/org/fcrepo/http/commons/api/rdf/HttpResourceConverter.java#L155

Where the URI matched the template, but there wasn't a path value. Since I'm not operating in a web context, I can't create a UriBuilder with the JAX-RS context. So I suspect there is some other way to create a UriBuilder that would satisfy the requirements. Having that requirement documented in the constructor javadoc would be good for future users of this class, too.

That all makes good sense, @escowles. I believe @ajs6f is wanting some javadoc-umentation describing something about the use and assumptions of the identifiers that would be created by the new constructor on DefaultIdentifierTranslator. My feeling is that a common, documented understanding is desired, not necessarily a different implementation.

@awoods, I've updated the Javadoc comments to better explain the purpose of the class, differentiate the constructors from each other, and link to HttpResourceConverter.

Thanks, @escowles. The docs look good. Hopefully @ajs6f will provide comments shortly.

In fact, now that I understand what the desire is I don't agree that this is a good change to the class (instead of a new class) but I don't think the difference is worth troubling about.

@ajs6f, would your suggestion be to create a new class that extends DefaultIdentifierTranslator?

The other way around-- to create a class for what @escowles very reasonably wants to do, something like PrefixingIdentifierTranslator, and let DefaultIdentifierTranslator inherit from that.

OK, @ajs6f @awoods, I've created a new PrefixingIdentifierTranslator and made DefaultIdentifierTranslator extend from that.

Thanks. As a TODO to myself, much of what happens with chains of translators inside these converters should be factored out into some abstract class, or post Java 8, default implementation.

Resolved with: 2c3eb0f