-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
render API simplification #193
Comments
Thought about it a little bit longer - probably I hadn't internalized the new |
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. |
Ok, Scott, what about the following:
Sounds feasible 😄 |
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 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:
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. |
I'm not sure if I fully understand your concerns. For the caller I have this in mind (supposed there would a factory method
So the implementation class for 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. |
Alright: here is my sketch: obecker@bfcac49 It's not quite the original idea because of the
(edit one hour later ...) By the way, instead of the
(with an additionally static or even more simplified
(apart from specifying the indent string I cannot imagine other useful |
I think I understand your goal now. Please continue to refine it. I'd personally keep the distinct |
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 In the long term I think we should get rid of the 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 Finally I tried (for fun) to remove the |
Sorry for haven't checked this earlier, but after using the 1.5.0 version I noticed that the
render
API might need one more simplification.Background: I have a boolean variable that controls whether the output should be flat or indented.
Creating a String is as simple as this:
However, I want to render the HTML into a file without creating a String, and unless I missed something the new API requires doing this:
and if there's also a
Config
instance involved, it's even more verbose:I wonder if we could introduce some kind of factory that would allow writing this:
The simplest version I can think of could be something like:
and
but that's probably not very user/developer friendly, as this
BiFunction
doesn't not provide any help what value to pass here. Perhaps an explicitHtmlBuilderFactory
would be clearer.What do you think, @sembler ?
The text was updated successfully, but these errors were encountered: