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

Closure style invocation doesn't return closure #66

Closed
udoprog opened this issue Jun 27, 2018 · 6 comments
Closed

Closure style invocation doesn't return closure #66

udoprog opened this issue Jun 27, 2018 · 6 comments

Comments

@udoprog
Copy link
Contributor

udoprog commented Jun 27, 2018

Hey!

Follow up from #62 and the changes done after that was merged.

Now the proptest! when invoked like a closure, doesn't actually return a closure. This is a bit unexpected. For example, the move |..| {} variant of the macro doesn't do anything special even though it might seem like it does.

Basically it runs the property test in-place, like a block statement.

This makes the thing I initially wanted to accomplish in #62 a bit more awkward, instead of:

runner.test("some test", proptest!(|..| {
    /*  */
}));

I now have to define an outer closure:

runner.test("some test", || {
  proptest!(|..| {
    /*  */
  })
});

If possible I would like to have a way that permits me to define closures with as few brackets as possible.
If there's a need for block-style invocation, that should preferably have none-closure style syntax since it's a bit misleading.

Thanks!

@AltSysrq
Copy link
Collaborator

To be frank, I'm quite confused about what you're looking for now. Your original PR called runner.run() within the macro.

runner.test

Is this referring to some other test framework? If so, your example could just be written

runner.test("some test", || proptest!(|..| {
  /* ... */
}));

@udoprog
Copy link
Contributor Author

udoprog commented Jun 28, 2018

Hey @AltSysrq

To be frank, I'm quite confused about what you're looking for now. Your original PR called runner.run() within the macro.

You are missing this part which was removed here. The PR had proptest! return a closure which you have to capture and call yourself. After your change it no longer returns a closure, it just evaluates the test in place. Given your comment, I'm not sure this was intentional.

Is this referring to some other test framework? If so, your example could just be written.

Yes, the test framework I use captured the closure in order to execute it as part of a suite. Now there is a fair bit more bracket matching to be done because I have to wrap everything in closures manually.

@AltSysrq
Copy link
Collaborator

The PR had proptest! return a closure

Yes. I discovered that when trying to document what that usage style did and was so suprised that it was returning a closure that I assumed it was an error.

I don't think returning a closure is a good fit for a general-purpose macro here. Users trying to use it in a normal context would need extra syntax to just invoke the closure immediately after (most likely) finding out the hard way that the macro did absolutely nothing without the extra (), whereas making closures around normal immediate code is something everyone does every day.

Unless there's something else I'm missing, it seems to me that the new form is enough of a building block to just make your own convenience macro.

macro_rules! ptc {
  ($($t:tt)*) => { || proptest!($($t)*) }
}

@udoprog
Copy link
Contributor Author

udoprog commented Jun 28, 2018

Ok!

I don't think returning a closure is a good fit for a general-purpose macro here.

I think the pertinent part here is that the macro invocation looks like a closure. And folks seeing it will likely expect it to behave like a closure. I believe it's considered good practice to try to mimic existing language items when building macros.

If we are after something that performs in-place execution, something that doesn't look like a closure is probably more suitable, like:

proptest!((...) {
})

Unless there's something else I'm missing, it seems to me that the new form is enough of a building block to just make your own convenience macro.

I already have. It's just a bummer that I have to :/.

@AltSysrq
Copy link
Collaborator

the macro invocation looks like a closure

I guess we have different ways of looking at things; I saw it as a function-like macro that mimics a function taking a closure as an argument. (I was wondering why you used braces though; now that makes sense.)

I suppose we'll just have to see how it plays out over time now that it's been released in its current form.

@udoprog
Copy link
Contributor Author

udoprog commented Jun 28, 2018

Ok, let's let it settle for a while. Thanks for explaining your reasoning!

@udoprog udoprog closed this as completed Jun 28, 2018
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