CONFLUENCE-240: Make AbstractMacroConverter and the Confluence Converter public #70
Conversation
…ter public * Introduce the public AbstractMacroConverter class
…ter public * Make most macro converters use the new class
…ter public * Make a public ConfluenceFilterReferenceConverter interface in Confluence-XML * Make ConfluenceConverter an implementer of this interface
…ter public * Make most things use ConfluenceFilterReferenceConverter instead of ConfluenceConverter
…ter public * Mark ConfluenceConverter as deprecated so IDEs can warn against injecting it
raphj
requested review from
tmortagne,
Josue-T,
trrenty,
Farcasut and
IdentityOverflow
| /** | ||
| * Base class for {@link MacroConverter} implementations. | ||
| * @version $Id$ | ||
| * @since 9.82.0 |
There was a problem hiding this comment.
Shouldn't it be something like 9.83.0 ?
There was a problem hiding this comment.
I'll make sure the since annotations are up to date when I merge this
|
|
||
| private void warnCantMark() | ||
| { | ||
| logger.error( |
There was a problem hiding this comment.
The method name suggest the intent is .warn, but maybe the method name is wrong ?
There was a problem hiding this comment.
Indeed. I'll rename this to logCantMarkError or something like this
| /** | ||
| * @param name the name to validate | ||
| * @return the validated name | ||
| * @since 9.26.0 |
There was a problem hiding this comment.
The @since from the methods don't really make much sense anymore, since they are lower than the class's @since.
| @Singleton | ||
| public class ConfluenceConverter implements ConfluenceReferenceConverter | ||
| @Deprecated (since = "9.83.0") |
There was a problem hiding this comment.
Do you need to keep this component since it's internal ? Don't want to fix all the code that calls it just yet ?
There was a problem hiding this comment.
@tmortagne that's the tricky part
- We need this component as it's the implementation of
ConfluenceReferenceConverter, so we need to keep it. We could rename it to DefaultConfluenceReferenceConverter, but... - This component has been injected in other projects providing macro converters (including code we don't control). That's my mystake of not handling CONFLUENCE-240 earlier. I would like to not break them (now is a very bad time for this) and leave a few months of transition as things settle, after which we can rename ConfluenceConverter to DefaultConfluenceReferenceConverter and remove the deprecated annotation
I'm preparing to document various things on the confluence package. Before I can do this, some things need to be cleaned up so we don't document shit, and in particular this means addressing CONFLUENCE-240: we now want external projects to be able to write macro converters, and this means making AbstractMacroConverter and the Confluence reference converter public and not internal.
I also want to deprecate toParameterName and toParameterValue, as we now want to better control what parameters get passed when converting macros. We don't want confluence parameters to leak by default anymore because it may cause compatibility issues later (it's still a good strategy for bridges, but this is handled by DefaultMacroConverter, custom macro converters don't need this)
Please let me know if you see something wrong with this.
I also wanted to deprecate using ConfluenceURLConverter from the XHTML package directly as a role and move the role in confluence-xml as a separate, dedicated, interface, because fundamentally implementers of URL converters should not care about the specific Confluence syntax that is used, but didn't manage to do this cleanly without breaking backward compatibility, and I guess it doesn't matter too much in practice.
Assigning a few people who have been dealing with macro converters for review but feel free to ignore. It might be the best occasion to address some pain points you might have encountered as we are introducing a new interface and can amend a few things without breaking stuff.