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

Ability to consolidate So statements with parent convey text #49

Closed
joliver opened this issue Oct 17, 2013 · 8 comments
Closed

Ability to consolidate So statements with parent convey text #49

joliver opened this issue Oct 17, 2013 · 8 comments

Comments

@joliver
Copy link

joliver commented Oct 17, 2013

I'm wondering if--and this is a big i" depending upon the intention and best practices of the library--having the ability to condense the So statement down to a single line along with the parent Convey text. For example, it may be that there is a lot of assertion blocks that take the following form:

Convey("The value should be computed correctly", func() {
    So(a, ShouldEqual, b)
})

Would it be possible to consolidate to rewrite the following in this way or something like it?

So(a, ShouldEqual, b, "The value should be computed correctly")

Perhaps this pollutes the DSL too much and perhaps it's mixing the concerns--that of conveyance of the intention and the underlying assertions. The only possible benefit is the slight reduction in code.

Thoughts?

@ghost ghost assigned mdwhatcott Oct 17, 2013
@mdwhatcott
Copy link
Collaborator

I've actually thought about this but I didn't have any brilliant ideas at the time so I dismissed it. I definitely think that the conveyance of the idea should come before the assertion because that makes it read better and I think that's what the developer should expect so I don't like the idea of appending a message parameter to the So method.

But seeing your ideas @joliver sparked one of my own which might be pretty simple to implement. How about this:

Convey("The value should be computed correctly", So, a, ShouldEqual, b)

It would be up to the discover method to sort out all the parameters received by Convey and then just create a func that passes a, ShouldEqual, and b into So at the appropriate time in the process.

There are only 2 downsides to this approach that I can think of.

First, it reminds me of languages like C# or javascript that allow you to omit curly braces for a compound statement if there's only one statement in the body. It's great when there really is only one statement because the code is cleaner but it's a pain to expand the statement back to the full form when another statement comes along (which I guess is why Go forces you to put curly braces in all the time for compound statements).

Second, this does sort of cloud the public API in that there are so many ways to call Convey:

  • with *testing.T
  • without *testing.T
  • with nil (if it's not implemented yet)
  • with a func() that has assertions or additional Convey calls
  • and now possibly with So and all of it's potential parameters

On the plus side, it would encourage a single assertion per Convey which is probably a good convention to follow and it looks nice. :)

I don't think it would be hard to implement because we already accept an arbitrary collection of inputs to Convey, it just creates more options for the developers using GoConvey and so a bit more of a learning curve.

But having said all of that I think I like the idea.

@joliver
Copy link
Author

joliver commented Oct 17, 2013

I wasn't sure which side to put the text message on--before or after the So values, but it definitely looks better at the start. You're right that it makes expanding an existing Convey more difficult when adding additional So statements, but that's not a huge downside.

@mholt
Copy link
Contributor

mholt commented Oct 24, 2013

Mostly done. Just gotta make sure the reports give the right line number...

@mholt
Copy link
Contributor

mholt commented Nov 8, 2013

So, this is turning out to be pretty gnarly (see the dsl branch)... wondering if it's really going to be worth our time, and even a good idea. Personally I'm leaning toward keeping it the way it is for consistency and simplicity's sake. (I know we discussed at length two good variations of the consolidated So statements, but maybe that means it's too subjective?)

@mdwhatcott
Copy link
Collaborator

@mholt it is gnarly at the moment. In the back of my mind I'm thinking about how to improve the internal design of the framework to better accommodate this feature, provided that we can eventually come to agreement about the DSL/syntax. So we could remove the branch with the expectation that we'll approach this later from a fresh perspective or just let the branch persist off to the side while the ideas bounce around our minds for a while.

@mdwhatcott
Copy link
Collaborator

Not quite sure how to go about this yet so we are deferring this issue. It will be referenced on the wiki for future discussion.

@mdwhatcott
Copy link
Collaborator

@smartystreets - As a result of solving issue #70 this would be a piece of cake to implement. But, the question still remains, what is the preferred syntax, and does it cloud the Convey API? Any thoughts?

@mholt Because the repository and the model have changed quite a bit since we created the dsl branch (it's over 200 commits behind) I'm going to remove it. I'm confident that we can get this working very quickly in a new branch if we so choose.

@mholt
Copy link
Contributor

mholt commented Feb 1, 2014

Personally, I like the DSL better without the combined So statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants