[MAGNOLIA-2828] Node Builder Task Created: 31/Jul/09  Updated: 23/Nov/09  Resolved: 14/Oct/09

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 4.2
Fix Version/s: 4.2

Type: Improvement Priority: Major
Reporter: Norman Wiechmann Assignee: Magnolia International
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Java Source File CreateConfigNodeTreeTask.java     Java Source File CreateNodeTreeTask.java    
Issue Links:
supersession
is superseded by MAGNOLIA-2897 Introduce a simpler api to build node... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

I found a comment by Gregory regarding simplified Node Builder API in the wiki.

"I'd like to propose a simple DSL/API similar to the one used to build tasks and deltas for this."

When we had to configure the STK programmatically I created a Task to do node manipulation using a fluent language style. I would like to donate the code to the project if you like it. The following example will illustrate the usage of the Task:

import static com.aperto.magkit.module.delta.CreateConfigNodeTreeTask.*;
...
private final Task _configurateTemplateStkSection =
    selectConfig("Config Task stkSection", "Configure stkSection template.", PATH_STK_TEMPLATES,
        select("stkSection/mainArea",
            setProperty("template", "/templates/xxx/pages/section/pageIntroMainArea.ftl"),
            createNode("floating", setProperty("columns", "2"), setProperty("enabled", "true")),
            createNode("opener/paragraphs/xxxTeaserOpener", setProperty("name", "xxxTeaserOpener")),
            createNode("paragraphs",
                createNode("xxxSingleLink", setProperty("name", "xxxSingleLink")),
                createNode("stkTeaserFingerTabbed", setProperty("name", "stkTeaserFingerTabbed")),
                createNode("xxxTeaserNewsList", setProperty("name", "xxxTeaserNewsList")),
                createNode("xxxExternalTeaser", setProperty("name", "xxxExternalTeaser")),
                createNode("xxxChronicleTeaser", setProperty("name", "xxxChronicleTeaser"))),
            remove("opener/paragraphs/stkTeaserOpener")));

Find the source of the Task is attached to the issue.



 Comments   
Comment by Magnolia International [ 31/Jul/09 ]

Awesome, thanks !
Will come back to you once 4.1.1 is out the door; we'll probably want to change a few things here and there, let's discuss.

Comment by Magnolia International [ 09/Oct/09 ]

Guys,

Using a different approach, this is the syntax I came up with:

        final Task configurateTemplateStkSection = new NodeBuilderTask("Config Task stkSection", "Configure stkSection template.",
                "config", "/modules/stk/templates") {
            protected void doExecute(NodeBuilder nb) throws RepositoryException {
                nb.get("stkSection").get("mainArea")
                        .setProperty("template", "/templates/xxx/pages/section/pageIntroMainArea.ftl")
                        .add("floating")
                        .setProperty("columns", "2")
                        .setProperty("enabled", "true")
                        .parent()
                        .get("opener").get("paragraphs").get("xxxTeaserOpener")
                        .setProperty("name", "xxxTeaserOpener")
                        .parent().parent().parent()
                        .add("xxxSingleLink").setProperty("name", "xxxSingleLink").parent()
                        .add("stkTeaserFingerTabbed").setProperty("name", "stkTeaserFingerTabbed").parent()
                        .add("xxxTeaserNewsList").setProperty("name", "xxxTeaserNewsList").parent()
                        .add("xxxExternalTeaser").setProperty("name", "xxxExternalTeaser").parent()
                        .add("xxxChronicleTeaser").setProperty("name", "xxxChronicleTeaser").parent()
                        .get("opener").get("paragraphs").remove("stkTeaserOpener");
            }
        };

The NodeBuilder interface looks like this (I suppose it's pretty obvious that calls to add() and get() return subnodes, while others return "this".

interface NodeBuilder {
    NodeBuilder add(String name);
    NodeBuilder add(String name, String type);
    NodeBuilder get(String name);
    NodeBuilder remove(String name);
    NodeBuilder parent();

    NodeBuilder addProperty(String name, Object value);
    NodeBuilder setProperty(String name, Object newValue);
    NodeBuilder setProperty(String name, Object expectedCurrentValue, Object newValue);
    NodeBuilder removeProperty(String name);
}

Clear advantages for yours: indentation provided by IDEs code layout give some obvious readability: the dot-based syntax gets all calls aligned horizontally, so it's hard to "visualize" the hierarchy.

I'm under the impression that my approach is much simpler to implement (which of course is a non-argument since you've done much of the work already - but we also have to take testability into account)

I'm not convinced by the calls to parent() that are necessary to achieve the same as your example; of course, one could introduce variables in this code, removing the need for calling parent() all the time; on the other hand, the parenthesis/coma-based syntax can also be hard to follow when nesting a bunch of elements.

Thoughts ?

Comment by Philipp Bärfuss [ 12/Oct/09 ]

I don't have a strong opinion. But I found a helpful article explaining the different flavors of DSL API:
-> http://martinfowler.com/dslwip/InternalOverview.html

A) Original Solution (Nested Functions)
readable due to automatic indentation
more complex implementation due to having to build an operation tree
not so easy to understand as it builds a operation tree which is
executed later: debugging, ..

B) Gregory's Approach (Builder)
easy to understand
code auto completion
without indentation not really readable

Note: what I like about the operation tree that this can be used to create task descriptions, but B) can also be implemented in a way that it builds a n operation tree instead of executing the operations immediately.

Comment by Jan Haderka [ 12/Oct/09 ]

Would not changing the B) to build op tree bring in also the disadvantage of complicating the debugging?

Comment by Magnolia International [ 13/Oct/09 ]

Yet another possibility/approach

import info.magnolia.fluent.x.NodeBuilderTask;
import static info.magnolia.fluent.x.Ops.*;
import info.magnolia.module.delta.Task;

        // ExampleC 
        final Task configurateTemplateStkSection = new NodeBuilderTask("Config Task stkSection", "Configure stkSection template.",
                "config", "/modules/stk/templates",

                get("stkSection/mainArea").then(
                        setProperty("template", "/templates/xxx/pages/section/pageIntroMainArea.ftl"),
                        add("floating").then(
                                setProperty("columns", "2"),
                                setProperty("enabled", "true")),
                        add("opener/paragraphs/xxxTeaserOpener").then(
                                setProperty("name", "xxxTeaserOpener")),
                        add("paragraphs").then(
                                add("xxxSingleLink").then(
                                        setProperty("name", "xxxSingleLink")),
                                add("stkTeaserFingerTabbed").then(
                                        setProperty("name", "stkTeaserFingerTabbed")),
                                add("xxxTeaserNewsList").then(
                                        setProperty("name", "xxxTeaserNewsList")),
                                add("xxxExternalTeaser").then(
                                        setProperty("name", "xxxExternalTeaser")),
                                add("xxxChronicleTeaser").then(
                                        setProperty("name", "xxxChronicleTeaser"))),
                        remove("opener/paragraphs/stkTeaserOpener"))
        );
  • we have a tree of operations - while this is slightly less intuitive to debug, that brings self-describability - we could for instance generate complete english description sentences from a bunch of operations
  • no mixing of current operation arguments with children operations as with the original proposal (vararg argument replaced with intermediate then() method)
  • correct indentation in IDEs
  • still no auto completion due to the static nature of the get/add/set methods, although one could also insert custom Operation implementations in there
  • implementation is still fairly simple
Comment by Philipp Bärfuss [ 13/Oct/09 ]

Not bad indeed. the auto completion is semi problematic as one could still do:

add('name').setProperty(..).setProperty(...)

There are two things I would want to improve:

  • either use addNode() and setProperty(), or simply add() or set(), but we should not mix the schema
  • then() should be renamed to something like apply() because then might be mixed up with a if-then pattern.
Comment by Magnolia International [ 14/Oct/09 ]

See MAGNOLIA-2897.

Generated at Mon Feb 12 03:40:30 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.