robstoll / atrium-roadmap

Contains issues created by the maintainers of Atrium
1 stars 0 forks source link

shortcut / different fun for withOptions(textRepresentation=...) #72

Closed robstoll closed 4 years ago

robstoll commented 4 years ago

initial discussion https://github.com/robstoll/atrium/issues/153#issuecomment-582868750

matejdro: Isn't that syntax a bit too verbose? Maybe withDescription("xy numbers") would be better?

robstoll commented 4 years ago

@matejdro Thanks for your input, it's experimental so yes, we can definitely discuss about it. First a word to the naming in atrium:

expect: 1
* to be: 2   (kotlin.Int<123>)

to be is the description, 2 (kotlin.Int<123>) is the representation. It is up to the user if he wants to use withOptions("xy numbers") or withOptions(textRepresentation="xy numbers") but maybe withOptions("xy numbers") is actually a bad idea and something else would be better. Maybe withTextRepresentation("xy numbers") instead? I am using Text as you can also define something like list.filter { it % 2 == 0} as representation. Thoughts?

jGleitz commented 4 years ago

I agree that it’s a problem that users could do withOptions("xy numbers")

matejdro commented 4 years ago

To me, withOptions("xy numbers") is not very descriptive. If I do not know in advance what this method does, I would be confused. withTextRepresentation is better but IMO still a bit too long.

robstoll commented 4 years ago
matejdro commented 4 years ago

withDescription("xy numbers") sounds pretty good alternative to me.

robstoll commented 4 years ago

I thought that you are going to say that, would mean a lot of changes. Personally I have never had the use for it but inside of assertion functions. Thus I don't think it is bad if withTextRepresentation is a bit longer than withDescription. Can you please elaborate for what you require withDescription, this way I would better understand your need

matejdro commented 4 years ago

It is just personal preference. But yeah, it is minor difference, so go for the withTextRepresentation if it means much less work.

robstoll commented 4 years ago

another possibility would be to have two overloads:

withRepresentation(any: Any)
withRepresentation(s: String) = withRepresentation(RawString.create(s))

And one has to cast a String to Any in case he does not want it to be text. I think that would be valid as if you change the representation, then you are not really interested in things like identity hash etc.

jGleitz commented 4 years ago

I strongly feel that withDescription is inferior, simply because “description’ is less precise than “representation”

I like the idea of overloads, but I would do it differently:

This way, we don’t have overlapping definitions. If somebody has a constant non-string representation, they can just do

expect(6).withRepresentation { 5 }

but we also support:

expect(listOf(1,2,3,4,5,6,7,8)).withRepresentation { it.take(3) }

…and it is shorter since @matejdro is concerned about length (which I am not)

jGleitz commented 4 years ago

If we go with my suggestion, the docs should explain the subtle difference between withRepresentation { "foo" } and withRepresentation("foo").

robstoll commented 4 years ago

Let's put that in the context that a subject does not need to be defined. For a root expect this shouldn't be the case but for a feature assertion this could be:

expect(listOf(1,2)).get(99).withRepresentation { "my lovely number $it" }

Currently we have withSubjectBasedRepresentation for this case. I wouldn't mind if we get rid off SubjectBased but then we need another overload for the case where a representation is not based on the subject. Unless... we always show an error message (https://github.com/robstoll/atrium#within-assertion-functions and don't provide a way to define a constant representation in case of an non-defined subject -- right now I think this might actually be better

jGleitz commented 4 years ago

I agree that we just don’t need to offer a way to change the representation of a non-existant subject. For me, withRepresentation has implicit withRepresentationIfSubjectExists semantics.

robstoll commented 4 years ago

@matejdro what do you think about jGleitz proposal?

matejdro commented 4 years ago

I think it's good, although I do not completely understand the idea of non-String representation (just started using the library). Isn't the whole point of it printing to the console?

jGleitz commented 4 years ago

I also had to learn that, but it actually makes sense. If you provide a representation, it will usually be printed with additional information, like 10 (kotlin.Int <1234789>). That is what will happen if you use the lambda version. So if you do expect("foo").withRepresentation { "bar" } you will get "bar" (kotlin.String <123456789>). But with expect("foo").withRepresentation("bar") you will just get "bar"

robstoll commented 4 years ago

You will even get bar without the "