Tree-like dsl for assertionBuilder

[vlsi]

Currently assertionBuilder uses fluent api. I am afraid it makes it hard to discover the api as the set of available methods strictly depends on the order of method calls.

Sample:

val inAnyOrderAssertion = assertionBuilder.list
            .withDescriptionAndEmptyRepresentation(description)
            .withAssertions(assertions)
            .build()

I think the api would be way easier without fluent bits.
For instance:

• explicit build is not required if assertion is created with lambda
• Description and representation could be two independent setters. Currently, one needs to adjust method name in order to pass or omit the representation
• The overall builder structure could better resemble the output report in case lambas are used for builders

Wdyt?

[robstoll]

can you make an example please?
the purpose of the builder is to have one way to write things and to have extension points. Also, this way we already know during compile time that all necessary data is set.

[vlsi]

Sample from

private fun <E> createAdditionalElementsAssertion(
    container: AssertionContainer<*>,
    expectedSize: Int,
    iterableAsList: List<E?>,
    itr: Iterator<E?>
)

Current:

val additionalEntries = itr.mapRemainingWithCounter { counter, it ->
    val description = TranslatableWithArgs(
        DescriptionIterableLikeExpectation.ELEMENT_WITH_INDEX,
        expectedSize + counter
    )
    assertionBuilder.descriptive
        .holding
        .withDescriptionAndRepresentation(description, it)
        .build()
}

assertionBuilder.explanatoryGroup
    .withWarningType
    .withAssertion(
        assertionBuilder.list
            .withDescriptionAndEmptyRepresentation(WARNING_ADDITIONAL_ELEMENTS)
            .withAssertions(additionalEntries)
            .build()
    )
    .failing
    .build()

Suggested:

assertionGroup {
    groupType = GroupType.EXPLANATORY // or INVISIBLE or whatever
    type = AssertionType.WARNING // or warning()
    failing()
    assertions {
        assertionGroup {
            description(WARNING_ADDITIONAL_ELEMENTS)
            assertions {
                itr.mapRemainingWithCounter { counter, it ->
                    descriptive {
                        holds()
                        description(
                            TranslatableWithArgs(
                                DescriptionIterableLikeExpectation.ELEMENT_WITH_INDEX,
                                expectedSize + counter
                            )
                        )
                        representation(it)
                    }
                }
            }
        }
    }
}

[robstoll]

I see several drawbacks of your suggestion:

• there is no straight forward way to extend the DSL. Take descriptive as an example. How would you hook into it and add debug information? With the current approach you can hook into every step and steer the builder into a different direction if needed. See for instance toBeWithErrorTolerance where withHelpOnFailureBasedOnDefinedSubject is used

• users will come up with illegal combinations and only discover it during runtime vs. currently compile time

  Description and representation could be two independent setters. Currently, one needs to adjust method name in order to pass or omit the representation

  I think that's a good example why the builder makes sense, you always need to define both, you cannot skip one of them (or we would need to define some default behaviour)

I am afraid it makes it hard to discover the api as the set of available methods strictly depends on the order of method calls.

This sounds like we should try to improve the naming so that it is clearer to the user what path it should choose. That's the current auto-complete suggestion (and it does not even show all 😢):

I totally get that a new user has no idea what the difference could be between explanatory and descriptive for instance. I would keep explanatory and explanatoryGroup but we should find a better name for descriptive IMO.

[robstoll]

I started to work on improved reporting again and now saw that back in 2021, when I actually started with the work I thought about throwing the builders all over board, make Atrium less extensible in this area and instead provide simple data classes which can be composed:
main...refactor-reporter#diff-7a8db870c94da6508d47da39895b8c328e46b51c5dda5f47240ab2a8a2c984d5R6

I kind of like the idea to drop the builders and instead provide those data classes. All will implement Reportable so they will not be too hard to find either + the defined data is very close to what you will see in reporting.
I can imagine that it will result in more ways how reporting will be structured which can be a bad thing in terms of consistency but also gives rise to other ideas which is a good thing. I will at least pursue this path to see how it works out

[vlsi]

users will come up with illegal combinations and only discover it during runtime vs. currently compile time

Frankly, the number of "valid and invalid" compile-time combinations is more-or-less the same when compared with "fluent API" style. The key difference I see is that with "fluent" you have exactly one explicit receiver that you mention before each dot (e.g. you call .descriptive on assertionBuilder) while with tree-like approach there might be several receivers in the scope. At the same time @DslMarker could help to make the unwanted descriptive { descriptive { ... } } a compile-time error.

there is no straight forward way to extend the DSL.
See for instance toBeWithErrorTolerance where withHelpOnFailureBasedOnDefinedSubject is used

I must be failing to understand, however, I do not see how tree-like DSL makes it hard to extend.
Users could supply their extension functions. For instance, Kotlin Flow operators can be extended with extension functions: https://elizarov.medium.com/simple-design-of-kotlin-flow-4725e7398c4c#f4b9

At the same time, I do not understand why do you think withHelpOnFailureBasedOnDefinedSubject (

atrium/logic/atrium-logic/src/commonMain/kotlin/ch/tutteli/atrium/logic/impl/DefaultFloatingPointAssertions.kt

Line 78 in 83147a3

.withHelpOnFailureBasedOnDefinedSubject(container.toExpect()) { subject ->

) would be especially problematic for tree DSL. It could be the same extension function or whatever.

---

I think that's a good example why the builder makes sense

Could you clarify what do you call a "builder"?
The thing is that both "fluent API" and "tree-like DSL" are builders. See https://kotlinlang.org/docs/type-safe-builders.html

---

I kind of like the idea to drop the builders and instead provide those data classes

Frankly speaking, I treat "data classes for intermediate representation" orthogonal to "API for building them".
For instance, there's buildString {...} and buildList {...} builders even though there is mutableListOf(..) function.
Sure having solid data class foundation can't be a mistake.

[robstoll]

Frankly speaking, I treat "data classes for intermediate representation" orthogonal to "API for building them".

Me too, this was not clear enough from my side. What I meant is that the cost (in terms of maintenance)/benefit ratio might not be high enough to keep a (tree-like or fluent) builder if the data structure offers more or less the same declarative style. There are drawbacks of course. Coupling is higher with data classes and they are very problematic when it comes to retain binary backwards compatibility but on the other hand they are easier to deal with when it comes to manipulation (map, filter etc.) and composing them. And of course no extra maintenance cost for the builders, that's a big win IMO. But yeah... I might still put something on top or use regular classes instead of data classes in the API due to backward compatibility reasons. I'll see but the more I think about it the more I tend to not go in a direction where holding binary compatibility could mean a lot of pain (I have dropped the bc-tests but thought about introducing https://github.com/Kotlin/binary-compatibility-validator instead and start guaranteeing binary backward compatibility between versions again)

The main difference between fluent-builder and tree-builder I see

fluent-builder

asssertionBuilder
  .descriptive
  .withTest(...)
  .withDescriptionAndRepresentation(...)
  .build()

asssertionBuilder
  .descriptive
  .build() // compile-time error, the method build does not exist at this point

I.e. fluent builder enforce a certain chain (in contrast to regular builders which usually return this)

tree-like builder are more like regular builders as you cannot enforce an order within your lambda

val assertion = asssertionBuilder {
  descriptive {
     withDescriptionAndRepresentation(...)
     // ... other options
    withDescriptionAndRepresentation(...) // does this override the previous definition?
  }
}

The above will only fail at runtime because withTest was not defined. I don't see how you could prevent that without ending up with a mixture of fluent- and tree-style builder or/and tree-like builder simulating/hiding data classes (e.g. as kotlinx.html does) something like:

assertionBuilder {
  descriptive(description =..., representation = ..., test = {}) {
    // add additions here like
    withHelpOnFailureBasedOnDefinedSubject(...)
  }
}

However, I don't see how withHelpOnFailureBasedOnDefinedSubject, being an extension, can influence how the Assertion is created in the end in the tree-like builder. With the fluent-builder pattern, withHelpOnFailureBasedOnDefinedSubject defines a custom final step and thus decides what is built in the end. In the tree-like builder you cannot have something like a final step as there is no defined order in which extensions are applied. Maybe you could come up with some decorator mechanism allowing extensions to transform the initial DescriptiveAssertion but I guess it would introduce more complexity compared to the fluent-builder. Yet, I am not sure we really need this extensibility, in this regard a tree-like builder would work as well. However, where is the added value compared to just classes? The beauty of composing just classes is that the extensions don't need to know something from each other. They create small pieces and can be composed. I tend to put the classes behind factory methods for BC reasons (and UX) and even provide functions for common patterns (a bit like predefined components if you think in UI terms).

The drawback of this approach, compared to the fluent builder pattern, is of course that you loose the pre-defined order. I.e. someone might as well compose the classes in ways not pre-seen or just in a different order than usually done in Atrium (e.g. putting the failure hint before the description/representation). Which means this will lead to different ways how reporting looks like and thus also how one needs to read reporting. I am not yet sure if I want to allow that. Having as few ways to do the same thing in Atrium is an advantage IMO.

[robstoll]

going to introduce a tree-like builder for creating Proofs, hence closing this discussion. Thanks for the input 👍