Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

withWindow(Map, Closure, Closure) always returns null #476

Closed
fettlaus opened this issue Mar 16, 2017 · 6 comments
Closed

withWindow(Map, Closure, Closure) always returns null #476

fettlaus opened this issue Mar 16, 2017 · 6 comments
Milestone

Comments

@fettlaus
Copy link

fettlaus commented Mar 16, 2017

def withWindow(Map options, Closure specification, Closure block) always returns null(despite its documentation telling otherwise) because it potentially applies the closure to multiple windows. This makes it quite unusable in Spocks expect: or then: blocks.

As changing the behaviour of this function may break comptibility, maybe we can work around this by introducing a new option value to only execute the closure on the first matching window. Something like options.first.

@erdi
Copy link
Member

erdi commented Mar 20, 2017

I can't imagine anybody depending on this method returning null so I would not worry too much about compatibility - it is a bug anyway as it does not work as documented.

I don't think returning the value returned by block for the first window matched by specification is the way to go as it is more limiting than returning a list with an entry holding a return value from block for each window matched by specification. Would a behaviour like that work for you?

@fettlaus
Copy link
Author

As an empty list evaluates to false I would prefer a list that only contains all non-false return values. That way we get an empty list in case none of the executions of the block evaluate to true. This should allow us to use Spocks imlicit assertions.

@fettlaus
Copy link
Author

Or maybe even better: Return a list of results if the executions of all blocks return non-false values. Otherwise return an empty list if even a single one evaluates to false.

That way we don't get a false positive result for this method as a whole.

@erdi
Copy link
Member

erdi commented Mar 23, 2017

I still believe that your suggestions are limiting - they fit only your use case. It should not be up to the framework to interpret the result of your call and change the returned value based on where you want to use the method from.

I think that we should return a list with an entry holding a return value from block for each window matched by specification regardless of whether the values are truthy or falsy. It's up to you, the caller, to then interpret the result and in your use case it would be as simple as calling any() on it.

@fettlaus
Copy link
Author

You are right. Maybe I wanted too much at once. So then we should go with the more generic and more explicit approach.

And nobody prevents me to simply wrap that method and get exactly what fits my use case, now that we get return values.

@erdi
Copy link
Member

erdi commented Mar 25, 2017

It's great that I managed to persuade you and we arrived at a common understanding with regards to this.

@erdi erdi added this to the 1.2 milestone May 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants