Hi devs,
I’ve created a JIRA issue at https://jira.xwiki.org/browse/XRENDERING-595 to introduce the concept of a Syntax Registry. The initial need came from https://jira.xwiki.org/browse/MARKDOWN-72.
I’ve pushed some code in a branch to show you what it could look like:
Please check the jira issue and let me know what you think about this idea.
Thanks
Hi Vincent,
I globally agree with a Syntax Registry concept.
Given the API you proposed on https://jira.xwiki.org/browse/XRENDERING-595?focusedCommentId=107408&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-107408
/**
* Automatically register one or several syntaxes into the Syntax Registry. The syntaxes will be registered when
* the implementing component is registered into the Component Manager.
*
* @since 12.10RC1
*/
@Unstable
@Role
public interface SyntaxRegistryInitializer
{
/**
* @return the list of syntaxes to register into the Syntax Registry
*/
List<Syntax> initialize();
}
I don’t really agree with calling this method initialize since AFAIU it only returns the list of syntax to initialize: it does not perform the initialization itself.
You also said:
Note: We might also need a SyntaxTypeRegistry (not sure yet)
what would be the difference between SyntaxRegistry and SyntaxTypeRegistry?
We can discuss the name. What do you have in mind? I tried to find better names and came short so far.
Ideas:
SyntaxRegistrySyntaxProviderSyntaxProviderSyntaxRegistratorSyntaxRegistryEntrySame as what we have now. SyntaxTypeRegistry would allow you to register SyntaxType objects and SyntaxRegistry would allow you to register Syntax objects.
EDIT: As I said I’m not sure we need one but without one it means several modules that provide different syntaxes for the same syntax type need to either have syntax type duplication or some common module or depend on one another (for example).
I was not talking about the name of the class but just of the name of the method.
We could just have a getSyntax in it: apparently we use that in MandatoryDocumentInitializer where there’s a getDocumentReference.
Indeed. Could actually make sense for dedup as you mentioned and also for other usecases such as allowing registering a variant only if a syntax type is already registered.
it’s the same. It’s important to have the class and method names aligned. A Provider provides, an Initializer initializes, a Factory creates, etc. IMO you cannot disagree about initialize() but agree that it’s called SyntaxRegistryInitializer.
I personally continue to prefer SyntaxRegistryInitializer since it follows a pattern we use in lots of places in XWiki (ContextInitializer, etc) and this is about initialization for the SyntaxRegistry.
+1 for the idea. I would use a single registerSyntax and a single unregisterSyntax method with varargs.
I don’t agree with that. It’s just about providing syntaxes for the registry and not at all initializing the registry. If an extension which contains syntaxes to register is installed they will end up in the registry that was initialized long ago. So -1 for SyntaxRegistryInitializer and List<Syntax> initialize() naming.
Thanks but I’ve proposed 5 other options above and you didn’t say if you prefer one of them or not. If you don’t like any please suggest one.
Taking into account your point, I think I like List<Syntax> SyntaxProvider#get() (implementing the javax.inject.Provider interface).
WDYT?
I read it as “this module provides these syntaxes”, which seems to fit nicely.
I would go for List<Syntax> SyntaxProvider#getSyntaxes(). I don’t really see the point of implementing javax.inject.Provider since it would not be used that way and I prefer a more explicit method name.
The reason is to stay with best practices and use well known existing interfaces instead of reinventing the wheel. Always best (since it means future code readers will understand it better since it’s well known and documented beyond xwiki)
This is not true actually. Anyone will be able to use it that way if they want it. some code using Guice (for ex), or CDI (another ex), can use it. It’s meant for that, to return a syntax.
That’s true only if we get those syntaxes through the Provider role, but I doubt that’s what we want here. I feel a more typed API would be safer/nicer.
There’s no need for a typed method name since it’s already contained in the class name. It’s actually creating a duplication. SyntaxProvider = provides syntax(es).
ok, can do, but the name would be plural then I guess: registerSyntaxes(Syntax...). It’s just slightly more painful when you have a List<Syntax>since you need create an array from it.
It’s not only about the method name, if you implement Provider you return an Object and since there is no point in using this through the Provider API I really don’t think implementing it is a good idea. People should use the registry to get syntaxes. We never said that a best practice for any component which want to provide stuff is to implement Provider.
You return a List<Syntax>: Provider (Java EE 6 )
For example:
...
public class SyntaxProvider implements Provider<List<Syntax>>
{
List<Syntax> get() {...}
}
BTW maybe it should be SyntaxesProvider instead of SyntaxProvider since we return a list.
This component will not just be for the Syntax registry. It’s a standard component that any code can call to list the syntaxes provided by a given module/component.
We never said that a best practice for any component which want to provide stuff is to implement Provider.
We do that in general since it’s about following well known java design patterns: Factory, Provider, Singleton, Builder, etc. For me Provider is almost a JDK class and I see it similar to the functional interfaces that exist in Java (Runnable and the like). I really don’t see why we would reinvent the wheel here (I don’t see a single advantage actually - the naming is not a real advantage since it’s already in the name).