don't distinguish between values and entries, use only plural form?

[robstoll]

I am currently implementing Map.contains and thus reconsidering if it is worth having the distinction between singular form and plural. Likewise for values vs. entries in Iterable.contains. Earlier on we even had objects but we already got rid of this one. I am not entirely sure yet what is better because the distinction has advantages as well.

Let's look at an example with Iterable.contains, the current situation:

expect(...).contains....value(1)
expect(...).contains....values(1,2,3)
expect(...).contains....entry { ... }
expect(...).contains....entries({ ... }, { ... }, { ... })

Instead of (I applied a DSL Marker - so that they appear most likely on top, I guess this is something we should do at some point):

how it could be afterwards

expect(...).contains....elements(1)
expect(...).contains....elements(1,2,3)
expect(...).contains....elements { ... }
expect(...).contains....elements({ ... }, { ... }, { ... })

The API would then look like the following (also added a DSL Marker):

We would have one version less (the singular value). We still need the singular entry if you like in order that one can write elements { } without the need for paranthesis.

Why did I introduce the distinction between values and entries? To avoid ambiguity problems. In theory one could have List<() -> Int> and would like to do something like:

expect(...).contains...elements({ notToThrow() }, { throws<IllegalArgumentException>() })

Which would fail due to the ambiguity problems. But I never had this use case so far myself and I think by now, that in such a case one can use the helper expectLambda (for the first lambda) to help the compiler to disambiguate

expect(...).contains...elements(expectLambda { notToThrow() }, { throws<IllegalArgumentException>() })

Somehow the current state is nice when reading it, it looks strange to have elements(1) IMO because it makes it look like one forgot to add at least a second value. Also the singular is nice in case of a single lambda.

So... after writing all this I tend to still use singular and plural but only one name. The bad news -- there was another reason why I made the distinction between value and entry. Kotlin 1.3.72 is not able to infer the correct type for:

expect(...).contains....element { }

It actually considers that the lambda could be E even though it never can The even worse news, this was not fixed with Kotlin 1.4 :cry: (https://youtrack.jetbrains.com/issue/KT-43619)

So all the writing for... nothing I guess, we have to stick with different names until JetBrains sorts this out

[jGleitz]

… but the only downside of having a singular and plural function is the slightly larger API surface, isn’t it?

[robstoll]

I can think of:

• API surface -- I don't mind, this is really a minor thing as it is only one function in addition
• code completion can be a tiny bit less handy as it might be that you want values but intellij suggests value -- but also this is neglectable IMO because you also need to choose the correct overload in case of only plural
• you need to change value to values in case you expect suddenly more -- also only a minor

The values vs. entries is more the part where I see problems sometimes but also only rarely -- in terms of: if the subject is a List then you can think of a List has entries, so why do I need to choose values ?. When it comes to map.contains on the other hand I am not sure if we should distinguish the two as well or not. But if we do in Iterable it probably makes sense to do in Map as well. Currently I have the following implementation for MapContains:

expect(..).contains...keyValuePair("a" to 1)
expect(..).contains...keyValuePairs("a" to 1, "b" to 1)
expect(..).contains...keyValue(KeyValue("a") { isLessThan(2) })
expect(..).contains...keyValues(KeyValue("a") { isLessThan(2) }, KeyValue("b") { isGreaterThan(2) })
expect(..).contains...keyValuePairsOf(otherMap)

I consider to have

expect(..).contains...entry("a" to 1)
expect(..).contains...entries("a" to 1, "b" to 1)
expect(..).contains...entry(KeyValue("a") { isLessThan(2) })
expect(..).contains...entries(KeyValue("a") { isLessThan(2) }, KeyValue("b") { isGreaterThan(2) })
expect(..).contains...keyValuePairsOf(otherMap)

and with Kotlin 1.4 even (if fixed as expected):

expect(..).contains...entry("a" to { isLessThan(2) })
expect(..).contains...entries("a" to { isLessThan(2) }, "b" to { isGreaterThan(2) })

Here I don't run into the same bug as with Iterable.contains and I could actually have only one word

[jGleitz]

I don’t understand. What is currently the difference of values vs entries? For a Map I’d prefer entries over keyValuePair by a lot, because entries is the language the Java API uses.

[robstoll]

Sorry for the confusion. values and entries is part of Iterable.contains. Values expects multiple E and entries multiple (Expect<E>.() -> Unit)? In terms of the java language we should use elements for Iterable/Collection and even for List (I thought they used entries for List, maybe an inconsistency they fixed by now)

I am also in favour of entries over keyValuePairs and keyValue for map. But it looks a bit inconsistent in respect with Iterable.contains. Because there we have the distinction.

One additional note, we only use one name for the shortcut functions -> contains one more reason to have it the same way in the long version. I'll check if I find a workaround for the problem entry { }

Following a few ideas:

• don't provide entry(e: E) only entries(e: E, vararg o: E) => the reason why we don't have the problem with contains
• provide elementWhich { } instead of element { } => different name no longer the problem
• check if @OnlyInputTypes would help (I doubt it though)

[jGleitz]

Can we rename entries to something like expected? I find entries very unintuitive for what it actually does.

expect(listOf(1,2)).contains.atLeast(1).expected { greaterThan(1) }

is more intuitive to my mind than

expect(listOf(1,2)).contains.atLeast(1).entries { greaterThan(1) }

The advantage would be that we can use entries for contains in Maps without having the inconsistency.

[jGleitz]

To make it fully fluent, one could decorate the assertions for expected:

expect(listOf(1,2)).contains.atLeast(1).expected { toBe(greaterThan(1)) }

[robstoll]

In other words, you prefer to have two different names (which one can be still discussed)?

[robstoll]

Ou man, the problem does not only occur for the singular version but also for the plural version :cry:

[jGleitz]

In other words, you prefer to have two different names

Ah, that was the question. Yes, I think I prefer different names. First, one might conceivably have an Iterable of Lambdas, which would make it ambiguous, second, I think it makes sense to differentiate the two forms in the code.

Nevertheless, I really think that the one taking assertion creators should have a better name than “entries”.

[robstoll]

IMO expected is not really better, it's too generic. How about the following for Iterables

... element(1)
... elements(1, 2,...)
... elementWhich { isLessThan(2) }
... elementsWhich({ isLessThan(2) }, { toBe(2) },...)

and in the same vain for Map:

... entry(1 to 1)
... entries(1 to 1, 2 to 5)
... entryWhich(1 to { isLessThan(2) })
... entriesWhich(1 to { isLessThan(3) }, 2 to { toBe(2) })

OK the version with 1 to { ... } will also not work in Kotlin 1.4, which means we still need a workaround as we have currently:

... entry(1 to 1)
... entries(1 to 1, 2 to 5)
... entryWhich(KeyValue(1) { isLessThan(2) })
... entriesWhich(KeyValue(1) { isLessThan(3) }, KeyValue(2) { toBe(2) })

[jGleitz]

...Which is also good. My main objection is that it is not really fluent.

[robstoll]

I see your point but it would mean we need to duplicate the same functionality just in order that it reads fluently and this could confuse as we have multiple times the same functionality. With the current approach, we can re-use the function we have with the drawback, that it does not always fit. Some more examples:

expect(listOf(1,2,3)).all { isLessThan(2) } // should be areLessThan(2)
expect(listOf("a", "b", "c")).all { startsWith("a") } // should be startWith("a")

Currently I don't see, how we could improve here without the unnecessary duplication. functions starting with is would be handleable. As you suggested, it could also be lessThan(2) instead of isLessThan(2) and I would then provide is and are as fillers so that one can write:

expect(1).is.lessThan(2)
expect(listOf(1,2,3)).all { are.lessThan(2) }

But things like startsWith etc. don't fit into this schema

[jGleitz]

I agree that at some point, fluency needs to be abandoned because adding alternatives just so it reads nice becomes ridiculous.

In atrium, I am (low key) annoyed that the most basic assertion uses “expect X to be” (which is grammatically correct) while other assertions use phrases like “expect X contains” or “expect X has” (which are not grammatically correct).

The former form has one more advantage: it stays the same in plural. So maybe we should discuss in a new ticket whether it is worth to use the “to + infinitive” form everywhere?

It would also solve the problem at hand, without needing to duplicate anything:

expect(listOf(1,2,3)).all {  toBeLessThan(2) }
expect(listOf("a", "b", "c")).all { toStartWith("a") }

To address one obvious concern: the functions would be getting longer. I find this fully acceptable. But if it would be considered a problem, we could do what Hamcrest does with its is function: Make to and toBe optional values to throw in between. A major disadvantage would be that there would be two ways to achieve the same thing: expect(1).lessThan(2) would behave just like expect(1).toBe.lessThan(2).

Without this last options, I would suggest something like

expect(listOf(1, 2)).toContain...elementsFound { toBeLessThan(3) }
expect(listOf("a", "b")).toContain...elementsFound { toStartWith ("a") }

with the last option, could become:

expect(listOf(1, 2)).to.contain...elementsWhich { are.lessThan(3) }
expect(listOf("a", "b")).to.contain...elementsWhich { startWith ("a") }

I like the first option better, to be honest. It's more straightforward, even though elementsFound is not the most common of wordings.

[jGleitz]

About the plural versions: I initially thought it wouldn't matter whether we have two versions. But now I realised that they don't make any sense grammatically: They would become plural based on the object oft the sentence. But that's wrong, they depend on the subject in the English language. I other words,

expect(listOf(1, 2)).contains.atLeast(2).elementWhich { lessThan(3) }

makes no sense.

To have grammatically correct sentences, we would need the singular and plural form with the same signatures, wouldn't we?

[jGleitz]

Personally, that is the issue I have with fluent APIs: in normal code, I don't care about (human) grammatic. It's not really spoken language. But one the API get's very close to being proper sentences, grammatic errors annoy me a lot. More than I care to admit 😄

I still love fluent APIs for testing, though.

[robstoll]

Thanks for your input. I have to say, my mind was still stuck with the state when toBe was actually the hack and not the correct grammar when we used assert as primary assertion verb instead of expect. Now that we promote expect as primary verb it could make sense to change everything to to...

I am not a native speaker, so I might be wrong, but I have the feeling both are correct because expect can be used differently with a subtle difference:

• I expect x to be less than 1 => Ich erwarte, dass x kleiner ist als 1
• I expect x is less than 1 => Ich nehme an, dass x kleiner ist als 1

If this is correct, then it basically depends on how you read expect(listOf(1,2,2)); if you read it as I expect of the list... or I expect the list...

For me the second is more fluent to read and thus I am reading it this way. No idea what others do...

Anyway, let's go through if the change to to... would make (more) sense than the current implementation. Good that we are not yet at 1.0.0. Otherwise, I would kind of feel bad if we are going to change almost all assertion function names. Pros:

• using to and notTo as prefix has some advantages IMO, as you always start with toor notTo you narrow the API surface and code completion will thus be better.
• it might be better suited in assertion creator lambdas (not entirely sure yet)

Cons:

• the name is longer but I am with you that this is negligible
• a lot of API changes - it really needs to be worth it though

Regarding plural vs. singular, I think here you are wrong. Using only plural would be wrong IMO but maybe you read it again differently than I do. Consider the following translations:

• I expect the list to contain at least once the element 1 expect(list).contains.atLeast(1).element(1)
• I expect the list to contain at least once the elements 1 and 2 expect(list).contains.atLeast(1).elements(1, 2)
• I expect the list to contain at least once an element which is less than 1 // no... here we would need to use is.. instead of to... expect(list).contains.atLeast(1).elementWhich { isLessThan(2) }
• I expect the list to contain at least once an element that I expect to be less than 2 // less fluent to read but grammatically correct I guess expect(list).contains.atLeast(1).element { toBeLessThan(2) } or maybe better (though more verbose) expect(list).contains.atLeast(1).elementThatExpect { toBeLessThan(2) } => too much IMO
• I expect the list to contain at least once some elements where I expect one of them to be less than 2 and another to be less than 3 expect(list).contains.atLeast(1).elements({ toBeLessThan(2) }, { toBeLessThan(3) })
• I expect the list to have elements where all of them are less than 4 // are... instead of to.. expect(list).all { areLessThan(4) }
• I expect the list to have elements where for all of them I expect them to be less than 4 // clunky but ok expect(list).all { toBeLessThan(4) }

Looking forward to your sentences :slightly_smiling_face:

[jGleitz]

Regarding grammatical correctness: I can’t find sources for my claims because I forgot all my education about technical terms in grammar. However, I am very sure that “I expect x is less than 1” is not correct. It should be “I expect that x is less than 1”. If you search for “expect” on the internet, however, all language guides suggest “expect to + infinitive”.

a lot of API changes - it really needs to be worth it though

Absolutely. I created #93 to evaluate whether the change is worth it.

Good that we are not yet at 1.0.0

Depends on your stance at versioning — whether incrementing the major version often is considered a bad thing or not.

---

Regarding plural vs. singular, I think here you are wrong. Using only plural would be wrong IMO but maybe you read it again differently than I do. Consider the following translations:

I never suggested that using only the plural would be correct! I only said that there are situations where using the plural would be necessary for grammatical correctness, even if there is only one object. I was slightly mistaken that it depends only on the subject, instead, it seems to depend on the subject and the object. In examples with atLeast(1), it depends only on the object, as your examples show.

However, if you have atLeast(2), you’d need a plural form that takes only one parameter: “I expect the list to contain at least two elements ‘3’”. However, I cannot think of a case where you’d need a singular form with more than one parameter.

Bottom line: The plural form should not be elements(first: T, vararg remaining: T) but rather elements(vararg expected: T).

[robstoll]

I never suggested that using only the plural would be correct!

Got you wrong in this regard and I see your point with atLeast(2) but I read it differently, so I most likely not going to change this. I read it as I expect the list to contain at least twice element 3 if you read it like this then it also makes sense with multiple elements and an assertionCreator:

• I expect the list to contain at least twice (each of) the elements 1, 2, 3
• I expect the list to contain at least twice an element that I expect to be less than 2

---

I think whether we are going to use elementWhich or something different depends heavily on #93, so I will wait here until we decided in #93.

[jGleitz]

The plural form should not be elements(first: T, vararg remaining: T) but rather elements(vararg expected: T).

That was rubbish. As long as you can write atLeast(2).elements(4), everything is fine. Is that possible?

[robstoll]

Currently it's not possible because we want that one passes multiple elements when using elements

[jGleitz]

Is there any disadvantage in making it possible (apart from element(1) being equivalent to elements (1))?

[robstoll]

Now, I wrote rubbish, it's already possible. What isn't is elements()

[robstoll]

Not necessarily related to this issue but since we discussed in this direction here I am going to ask here nonetheless. Following my current idea for Map.contains

expect...inAnyOrder.entry("a" to 1)
expect...inAnyOrder.entries("a" to 1, "b" to 2)
expect...inAnyOrder.entryWhere.key("a").value { toBe("2") }
expect...inAnyOrder.entriesWhere(
  { key("a") value { toBeLessThan(2) } }, 
  { key("b") value { toBe(3) } }
)

The benefit of splitting key and value in entryWhere is that someone could also come up with a different search algorithm for the key, e.g. via assertionCreator or even keyCanBeAnything etc. For instance:

expect...inAnyOrder.entriesWhere(
  { keyCanBeAnyhting value { toBeLessThan(2) } }, 
  { key { toStartWith("a") } value { toBe(3) } }
)

I will definitely not support this out of the box as I don't see use case myself but the design would at least allow it. I think that would be nice.

I only have some concerns with entriesWhere. a) it sounds a bit like I am going to state something which applies to all entries in the map. b) there are a lot of curly braces. Hence I was thinking about the following instead:

expect...inAnyOrder.entries {
  key("a") value { toBeLessThan(2) }
  key("b") value { toBe(3) }
}

Which is kind of a paradigm shift but I actually like it. What do you think? (I am also happy to name it entriesWhere)

[robstoll]

I quickly checked how the design would need to look like in order that someone could implement something like:

  { keyCanBeAnyhting value { toBeLessThan(2) } }, 
  { key { toStartWith("a") } value { toBe(3) } }

in the future. As far as I could see, reporting will suffer a lot if we want to have something that generic and therefore I will not support it. Instead, someone would need to use asEntries() and then they can basically do whatever they want and the reporting will look as expected (less nice).

[jGleitz]

Now, I wrote rubbish, it's already possible. What isn't is elements()

Good to hear! I don’t think that anybody needs elements, since you can simply use toBeEmpty().

[jGleitz]

Regarding entriesWhere: If it stays in its current form, I think this function should be named consistently with whatever we pick in #93 for elements. So currently, I would expect this to be named entriesWhichNeed. For me, consistency is important here.

However, I think that the proposal is verbose in its current form. I see where you are coming from, but I would like to at least try to do better. Can fuse what key … value … is doing into one function? I am think of something like:

expect...inAnyOrder.atLeast(3).toHave {
    entry("a"} { toBeLessThan(2) }
    entry("b") { toBe(3) }
}

I like this structure better, simply because it is shorter without losing information or precision (to my mind). I don’t like the name toHave all that much, though, it is very generic.

[jGleitz]

Technically, we could even do this, couldn’t we?

expect...inAnyOrder.atLeast(3).toHave {
    entry("a"}.toBeLessThan(2)
    entry("b").toBe(3)
}

[robstoll]

Can fuse what key … value … is doing into one function?

I should have mentioned that the status quo for the shortcut for Map.contains is:

expect(map).contains(KeyValue("a") { toBeLessThan(2) }, KeyValue(...))

So I think it makes sense if we stick with this instead of a new entry("a") { toBeLessThan(2) }

I would expect this to be named entriesWhichNeed

Fine with me

toHave

IMO doesn't fit because if we have a look at the full statement: expect(map).contains.inAnyOrder.toHave { ... }

Technically, we could even do this...

I guess this should be possible but... it might imply that we cannot tweak the reporting; we would need to stick to what we have defined elsewhere which also means . implies fail-fast behaviour and so it would influence reporting (not sure we could or should workaround this). For instance,

expect...inAnyOrder.entriesWhichNeed {
    key("a").toStartWith("a").toEndWith("b")
}

could look like the following in reporting if key "a" does not exist at all

expected that subject: {a=1, b=2}        (java.util.LinkedHashMap <1234789>)
◆ contains, in any order: 
  ⚬ ▶ entry "a": ❗❗ key does not exist

and if its value is -1

expected that subject: {a=1, b=2}        (java.util.LinkedHashMap <1234789>)
◆ contains, in any order: 
  ⚬ ▶ entry "a":  "bba"
      ◾ starts with: "a"        (kotlin.String <1234789>)

Which means I no longer have the context which I probably wanted. But... it could also be a nice thing. The user can decide on its own what he wants. Fail-fast or not.

[robstoll]

Althought I like the idea to have less curly-braces I am not yet convinced that it would be good, rather the opposite because it is not applicable to Iterable.contains where it would be ambiguous (only in terms of reading) where the definition of a new element starts and where the previous ends:

exepct(list).contains.inAnyOrder.atLeast(1).elementsWhichNeed {
    toBeLessThan(2)
    toBeGreaterThan(2)
}

and I would have again my previous concern:

a) it sounds a bit like I am going to state something which applies to all entries in the map

It would be even bigger with this one. We could have something like instead to work-around this:

exepct(list).contains.inAnyOrder.atLeast(1).elementsWhichNeed {
    element.toBeLessThan(2)
    element.toBeGreaterThan(2)
}

But IMO that is too verbose and also here I have the feeling it is better if we separate the definition for each entry by comma, i.e. have multiple arguments. The nice thing about having a block would be, that adding options like the one asked in https://github.com/robstoll/atrium/issues/292 would be easy -- i.e. would just be an additional method call. But adding an optional parameter will work as well.

[robstoll]

So after a while I am back at square one for Map.contains kind of. I think the best for the long form is as initially proposed:

expect(..).contains...entry("a" to 1)
expect(..).contains...entries("a" to 1, "b" to 1)
expect(..).contains...entry(KeyValue("a", { isLessThan(2) }))
expect(..).contains...entries(KeyValue("a", { isLessThan(2) }), KeyValue("b", { isGreaterThan(2) }))

The only thing I will change is keyValuePairsOf to entriesOf, makes more sense, even if one is allowed to pass a List<Pair<K, V> as well.

I am aware of that the name will not follow the schema in Iterable.contains where we plan to have elementWhichNeeds and elementsWhichNeed but entryWhichNeeds followed by a KeyValue just does not make sense.

[robstoll]

We could think about the following for Iterable.contains if we insist to have the same naming schema:

expect(list).toContain...element(1)
expect(list).toContain...element(1, 3)
expect(list).toContain...element(WhichNeeds { toBeLessThan(2) })
expect(list).toContain...elements(WhichNeeds { toBeLessThan(2) }, WhichNeeds { toBeLessThan(2) })

Personally, I would go with the two different names. We don't need to be that purist IMO

[jGleitz]

To be honest, I have lost track while thinking through all of these proposals. My gut feeling is that I like this form best:

expect...inAnyOrder.entriesWhichNeed {
    valueFor("a").toStartWith("a").toEndWith("b")
}

(note how I used the name valueFor to indicate that the assertion is for the value of the key "a"). I fully agree with your sentiment:

But... it could also be a nice thing. The user can decide on its own what he wants. Fail-fast or not.

If I don’t want fail-fast, I’ll write

expect...inAnyOrder.entriesWhichNeed {
    valueFor("a") {
        toStartWith("a")
        toEndWith("b")
     }
}

---

I propose to wait with this ticket until #93 is done. Mainly because I think that we should find a solution that is intuitive to write but also consistent with the rest of the API. Currently, there are too many factors for me to keep in mind in order to make a sound judgement.

[robstoll]

closing this for now, keeping singular and plural form