Closed vlsi closed 1 year ago
vlsi
commented
1 year ago It looks like
ActionChainBuilder would help to make the API easier to use (e.g. the builder might throw exception if no action was added)@CheckReturnValue might help to mark methods in jqwik that return important values that should not be discarded: https://youtrack.jetbrains.com/issue/KTIJ-7061
jlink
commented
1 year ago All „mutating“ methods of Arbitrary subtypes in jqwik return new instances, so the behaviour is consistent with the rest of jqwik. Would a different method name (eg withAction) be more revealing?
I didn’t know about CheckReturnValue and will have a look at it.
vlsi
commented
1 year ago Well, there's withMaxTransformations, so naming alone is probably not enough.
I guess the issue boils down to that I thought ActionChain.startWith was ChainBuilder while in practice it returns a free-floating arbitrary.
vlsi
commented
1 year ago I just checked, and adding @javax.annotation.CheckReturnValue on top of addAction does help in Java code:
For Kotlin, the issue is still unresolved. AFAIK the idea is to implement something like that at the language level.
jlink
commented
1 year ago Well, there's
withMaxTransformations, so naming alone is probably not enough.I guess the issue boils down to that I thought
ActionChain.startWithwasChainBuilderwhile in practice it returns a free-floating arbitrary.
A builder usually requires a final build() call, which makes the code more involved. That’s why I try to steer clear of builders where possible.
vlsi
commented
1 year ago A builder usually requires a final build() call, which makes the code more involved
Even though build() is verbose (well, Java is verbose anyway), explicit build() method provides a clear boundary when you consider the object to be fully configured.
In the case of chain arbitrary, it would be better to have validation during build(), so the errors like "please add at least one action, and remember to use the result of .addAction call" closer to the user code. Currently, the exception is thrown in the runtime from the deep internals of jqwik.
vlsi
commented
1 year ago Would a different method name (eg withAction) be more revealing?
It might indeed be a better idea to use withAction for mutation method.
For instance, with Arbitrary.filter, it is crystal clear that you expect that the original arbitrary is not modified, and the resulting is filtered. At the same time Arbitrary.addFilter would look like adding a filter to already existing arbitrary.
chain.withAction(..) would indeed suggest that the method produces new chain rather than mutates the old in-place.
jlink
commented
1 year ago Released in 1.7.2-SNAPSHOT
vlsi
commented
1 year ago @jlink , I suggest that type annotations should be in api scope, so they become transitive dependencies.
Even though Java specification says everyone should be ignoring the annotations that are not on the classpath, there are known issues when various compilers (e.g. Scala compiler) observe a classfile with a public method that is annotated with "unknown annotation".
That is basically the reason Guava exposes nullability annotations as transitive dependencies.
See https://github.com/pgjdbc/pgjdbc/issues/2053#issuecomment-775915780
jlink
commented
1 year ago @jlink , I suggest that type annotations should be in
apiscope, so they become transitive dependencies.
Will consult what others think and write about that. 1.8. at the earliest.
Current plan is to include annotations dependency in samples and in userguide recommendation.
Testing Problem
See https://youtrack.jetbrains.com/issue/IDEA-265263/Generalise-Stream-missing-terminal-operation-inspections-to-warn-about-missing-calls-to-terminal-operations-of-third-party#focus=Comments-27-4804303.0-0
Frankly speaking, I would expect that methods named like
addActionwould mutate the object, however, it turns outaddActiondoes not really mutate the chain.It turns out
addActionadds nothing, and the generated chain fails quite surprisingly in the runtime.