Move renderer access behind a renderer factory #2086
Conversation
Renderers are now accessed from a RendererFactory held by the package graph. This allows for changing renderers based on the configuration of DartDoc without models having to know anything about that.
| @@ -85,8 +85,8 @@ class CallableElementTypeRendererHtml | |||
| StringBuffer buf = StringBuffer(); | |||
| buf.write(elementType.nameWithGenerics); | |||
| buf.write('('); | |||
| buf.write(ParameterRendererHtml(showNames: false) | |||
| .renderLinkedParams(elementType.element.parameters) | |||
| buf.write(ParameterRendererHtml() | |||
There was a problem hiding this comment.
This instantiation is inside another renderer for HTML, and there isn't a way to get to the renderer factory from here. I think this is probably ok, but alternatively we could make CallableElementTypeRendererHtml take a ParameterRenderer as a constructor arg.
There was a problem hiding this comment.
Yes, I think HTML renderers calling/constructing each other is fine for now and could even be fine long term.
| @@ -51,9 +43,6 @@ class ParameterRendererHtml extends ParameterRenderer { | |||
| } | |||
|
|
|||
| abstract class ParameterRenderer { | |||
| bool get showMetadata; | |||
| bool get showNames; | |||
There was a problem hiding this comment.
My thinking here was that it will be easier to instantiate a singleton of this renderer (and others) if it doesn't have any constructor args, should that become a performance concern. It is a tad cumbersome to pass these around in method args, but the public renderLinkedParams() uses named params, so the call sites stay pretty clean.
Happy to revert this and let the renderer factory deal with instantiating with args if that's preferred.
There was a problem hiding this comment.
I think this is fine. If you're concerned about construction performance, the factory can always cache them and create pseudo-singletons.
| @@ -85,8 +85,8 @@ class CallableElementTypeRendererHtml | |||
| StringBuffer buf = StringBuffer(); | |||
| buf.write(elementType.nameWithGenerics); | |||
| buf.write('('); | |||
| buf.write(ParameterRendererHtml(showNames: false) | |||
| .renderLinkedParams(elementType.element.parameters) | |||
| buf.write(ParameterRendererHtml() | |||
There was a problem hiding this comment.
Yes, I think HTML renderers calling/constructing each other is fine for now and could even be fine long term.
| @@ -51,9 +43,6 @@ class ParameterRendererHtml extends ParameterRenderer { | |||
| } | |||
|
|
|||
| abstract class ParameterRenderer { | |||
| bool get showMetadata; | |||
| bool get showNames; | |||
There was a problem hiding this comment.
I think this is fine. If you're concerned about construction performance, the factory can always cache them and create pseudo-singletons.
Renderers are now accessed from a RendererFactory held by the package
graph. This allows for changing renderers based on the configuration
of DartDoc without models having to know anything about that.