render API simplification

[obecker]

render API simplification

[obecker]

Thought about it a little bit longer - probably I hadn't internalized the new HtmlBuilder concept yet.
I think the above sketch doesn't really simplify the API.
However, what we might consider is withdrawing the deprecation of render(Appendable) and add a default method for renderFormatted(Appendable) - but perhaps this is just my personal view biased by my current requirements.
Cheers, Oliver

[sembler]

I understand your perspective Oliver. It was a trade-off between flexibility or conciseness, and I chose the former under the assumption that users could have complicated formatting requirements that wouldn't fit within a more simplified API. I'll continue to think on this as well. Hopefully one of us will have a moment of inspiration.

[obecker]

Ok, Scott, what about the following:

• the j2html library provides an extension mechanism for users having custom or complicated formatting requirements, which is the HtmlBuilder interface
• the j2html library provides a (one) default implementation for this interface
• the decision between flat or indented HTML in this default implementation is controlled by a flag in the Config class
  (actually, the current Config has already an indenter which has no meaning for FlatHtml)
• that means the Config class would be solely responsible for the single DefaultHtmlBuilder, so we could also move all the configuration properties into this DefaultHtmlBuilder, thus simplifying the API further.
• so in the end both the Renderable and HtmlBuilder interfaces could stay as they are now,
  but FlatHtml, IndentedHtml, and Config would be merged into a single DefaultHtmlBuilder

Sounds feasible 😄

[sembler]

I'm not sure this would simplify the API. It would be nice to remove the need for a Config object/class, but that's not going to gain much when you want to configure the DefaultHtmlBuilder because you'd still need to support multiple factory methods or constructors in the API. Maybe not a permutation of the 5 different fields (minify?, Appendable, Indenter, TextEscaper, closeEmptyTags), but at least one for defaults, and one for the full set of fields that can be configured. From the users perspective that would mean sticking with the defaults, specifying each field during construction, or sorting through whatever other alternative factory methods exist. That probably won't save much typing at the call-site either. (Especially if we kept Config around since it would be required to choose between flat or indented HTML.)

I'm also hesitant to combine the builders. The logic for indentation is substantially different from flat rendering and adding more conditionals (to indent/newline or not?) would reduce maintainability. Additionally you'd still be left with the same problem we have now, i.e. some of the Config fields would go unused depending on which path the code follows. That's why I didn't combine them originally, and why I only used Config for bridging the old library versions to the new one.

Going off of your code examples it looks like the core difficulty is that there isn't an option to pre-configure the factory methods. I.e. you have to define both method calls as part of the conditional instead of saying , "Give me a factory that can create a wrapper around some Appendable later." I think that it would be nice to have something like:

// Scoped within a method, instance, or class...
Config config = Config.defaults() ... ;
ConfiguredHtmlBuilder format=  minify ? FlatHtml.preconfigured(config) : IndentedHtml.preconfigured(config));

...

// Scoped within a method.
html.render(format.into(out));

Let's try putting some of our ideas into code and see how they come out. If you're up for implementing your suggestions then I'd like to see them. I will do likewise.

[obecker]

I'm not sure if I fully understand your concerns.

For the caller I have this in mind (supposed there would a factory method HtmlBuilder.defaults()):

html.render(HtmlBuilder.defaults().into(out).indented(!minify));

So the implementation class for HtmlBuilder follows the same builder pattern methods as in Config (the methods might have a with prefix, but that's naming).

So IMHO for the caller this is much simpler.

What about the implementation side? Let's see ... I think I could easily create some facade that will use the current classes. I'll keep you informed.

[obecker]

Alright: here is my sketch: obecker@bfcac49

It's not quite the original idea because of the Appendable generics involved, but with this I can simplify my code to

html.render(HtmlBuilder.into(out).indented(!minify));

---

(edit one hour later ...)

By the way, instead of the indented configuration property, we should probably instrument the Indenter to do this. No indenter (null) would mean flat output, a configured non-null indenter would automatically mean with indent. So the line above would become

html.render(HtmlBuilder.into(out).indenter(minify ? null : Indenter.with("    ")));

(with an additionally static with method in the Indenter interface)

or even more simplified

html.render(HtmlBuilder.into(out).indent(minify ? null : "    "));

(apart from specifying the indent string I cannot imagine other useful Indenter implementations)

[sembler]

I think I understand your goal now. Please continue to refine it. I'd personally keep the distinct indent(boolean) and indenter(...) methods. I think you can drop the cssMinifier() and jsMinifier() methods as well.

[obecker]

Hi @sembler, while my first attempt had some rather major changes and introduced with the facade an additional step in the call hierarchy during the rendering, I stepped back and tried again with a slightly different approach. The new class is a builder in the GOF design pattern sense and returns either a FlatHtml or an IndentedHtml instance. I would suggest to hide these classes in the next major release (i.e. make them package private).

In the long term I think we should get rid of the Config class. Rendering specific configuration options can now be set by specific methods in the new DefaultHtmlBuilder. That's why I added @Deprecated here and there in Config.

I'm not sure how to continue with the CSS and JS minifiers. There are not used during rendering, but when constructing the HTML tree. So if we want to get rid of the global static fields, we need to pass them as configuration options when constructing the script or style tags.

Finally I tried (for fun) to remove the extends Appendable from the HtmlBuilder interface. It's not as easy as I thought because Attribute also implements Renderable, but this class needs a TagBuilder, not an HtmlBuilder. So perhaps we have to refine the current interfaces a little bit more before we can start a cleanup.