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

is(T)? #17

Open
dsaff opened this issue Jun 8, 2011 · 8 comments
Open

is(T)? #17

dsaff opened this issue Jun 8, 2011 · 8 comments

Comments

@dsaff
Copy link
Owner

dsaff commented Jun 8, 2011

In short, should we try to make sure that ASSERT.that(9).is("a") will not compile?

@cgruber
Copy link
Contributor

cgruber commented Jun 10, 2011

I want that. It's where I was hoping we take it.

@dsaff
Copy link
Owner Author

dsaff commented Aug 12, 2011

OK, looking on where the code has gotten to, I'm still not sold. A fair amount of code complexity is coming from generics, and half the complexity from generics is coming from supporting this one requirement.

But, at best, this can only give us some of the help we might hope for. For example, right now, this doesn't compile:

ASSERT.that(3).is(new Date());

But this does:

ASSERT.that(new Date()).is(3);

I think that getting people used to the idea that this is a compile-time safety net will lead to problems. For example, what if someone types (as they often do):

ASSERT.that(myServer.getUrl()).is("http://www.google.com");

This compiles, and returns "Not true that http://www.google.com is http://www.google.com"

This will cause a lot of head-scratching: it compiles, so the types must be right, so where is this error coming from? Only much later might someone realize that getUrl() returns a URL.

I'd rather people not depend on any compile-time help here, and instead give them a better error message:

Not true that URLhttp://www.google.com is Stringhttp://www.google.com

The only thing lost if someone happens to get the types wrong in a way this could have caught is that they run it one extra time, get the failure, and fix the test. It's true that we could go ahead and give them both a half-helpful compile-time check, and better error messages. But I still am not convinced this complexity is paying for itself, especially when I look at CollectionSubject, et al.

@cgruber
Copy link
Contributor

cgruber commented Aug 12, 2011

I'm a bit confused. I am not sure that ASSERT.that(myServer.getUrl()).is("http://www.google.com"); should compile. I think we want is() to be a stronger type than Object to avoid exactly this.

Christian.

On Aug 12, 2011, at 4:25 PM, dsaff wrote:

OK, looking on where the code has gotten to, I'm still not sold. A fair amount of code complexity is coming from generics, and half the complexity from generics is coming from supporting this one requirement.

But, at best, this can only give us some of the help we might hope for. For example, right now, this doesn't compile:

ASSERT.that(3).is(new Date());

But this does:

ASSERT.that(new Date()).is(3);

I think that getting people used to the idea that this is a compile-time safety net will lead to problems. For example, what if someone types (as they often do):

ASSERT.that(myServer.getUrl()).is("http://www.google.com");

This compiles, and returns "Not true that http://www.google.com is http://www.google.com"

This will cause a lot of head-scratching: it compiles, so the types must be right, so where is this error coming from? Only much later might someone realize that getUrl() returns a URL.

I'd rather people not depend on any compile-time help here, and instead give them a better error message:

Not true that URLhttp://www.google.com is Stringhttp://www.google.com

The only thing lost if someone happens to get the types wrong in a way this could have caught is that they run it one extra time, get the failure, and fix the test. It's true that we could go ahead and give them both a half-helpful compile-time check, and better error messages. But I still am not convinced this complexity is paying for itself, especially when I look at CollectionSubject, et al.

Reply to this email directly or view it on GitHub:
#17 (comment)

@dsaff
Copy link
Owner Author

dsaff commented Aug 15, 2011

So you're suggesting that public DefaultSubject that(Object target)
should be DefaultSubject that (T target), or did you have another
solution in mind?

David Saff

On Fri, Aug 12, 2011 at 5:06 PM, cgruber
[email protected]
wrote:

I'm a bit confused.  I am not sure that ASSERT.that(myServer.getUrl()).is("http://www.google.com"); should compile.  I think we want is() to be a stronger type than Object to avoid exactly this.

Christian.

On Aug 12, 2011, at 4:25 PM, dsaff wrote:

OK, looking on where the code has gotten to, I'm still not sold.  A fair amount of code complexity is coming from generics, and half the complexity from generics is coming from supporting this one requirement.

But, at best, this can only give us some of the help we might hope for.  For example, right now, this doesn't compile:

ASSERT.that(3).is(new Date());

But this does:

ASSERT.that(new Date()).is(3);

I think that getting people used to the idea that this is a compile-time safety net will lead to problems.  For example, what if someone types (as they often do):

ASSERT.that(myServer.getUrl()).is("http://www.google.com");

This compiles, and returns "Not true that http://www.google.com is http://www.google.com"

This will cause a lot of head-scratching: it compiles, so the types must be right, so where is this error coming from?  Only much later might someone realize that getUrl() returns a URL.

I'd rather people not depend on any compile-time help here, and instead give them a better error message:

Not true that URLhttp://www.google.com is Stringhttp://www.google.com

The only thing lost if someone happens to get the types wrong in a way this could have caught is that they run it one extra time, get the failure, and fix the test.  It's true that we could go ahead and give them both a half-helpful compile-time check, and better error messages.  But I still am not convinced this complexity is paying for itself, especially when I look at CollectionSubject, et al.

Reply to this email directly or view it on GitHub:
#17 (comment)

Reply to this email directly or view it on GitHub:
#17 (comment)

@cgruber
Copy link
Contributor

cgruber commented Aug 15, 2011

WAit... no... Maybe I'm misunderstanding the problem.

Oh... I see it now. The below code matches on that(Object) and is(Object). Hmm. I see. I was completely wrong, sorry.

At some level, this is a problem of URL's toString() sucking, frankly. I'm not sure how we solve that, unless in DefaultObject's is, we explicitly change the output text for failure to include the concrete type. Something like

"Not true that (URL) http://www.google.com is (String) http://www.google.com"

I think we can use Class.simpleName() for this visual disambiguation.

Christian.

On Aug 15, 2011, at 7:25 AM, dsaff wrote:

So you're suggesting that public DefaultSubject that(Object target)
should be DefaultSubject that (T target), or did you have another
solution in mind?

David Saff

On Fri, Aug 12, 2011 at 5:06 PM, cgruber
[email protected]
wrote:

I'm a bit confused. I am not sure that ASSERT.that(myServer.getUrl()).is("http://www.google.com"); should compile. I think we want is() to be a stronger type than Object to avoid exactly this.

Christian.

On Aug 12, 2011, at 4:25 PM, dsaff wrote:

OK, looking on where the code has gotten to, I'm still not sold. A fair amount of code complexity is coming from generics, and half the complexity from generics is coming from supporting this one requirement.

But, at best, this can only give us some of the help we might hope for. For example, right now, this doesn't compile:

ASSERT.that(3).is(new Date());

But this does:

ASSERT.that(new Date()).is(3);

I think that getting people used to the idea that this is a compile-time safety net will lead to problems. For example, what if someone types (as they often do):

ASSERT.that(myServer.getUrl()).is("http://www.google.com");

This compiles, and returns "Not true that http://www.google.com is http://www.google.com"

This will cause a lot of head-scratching: it compiles, so the types must be right, so where is this error coming from? Only much later might someone realize that getUrl() returns a URL.

I'd rather people not depend on any compile-time help here, and instead give them a better error message:

Not true that URLhttp://www.google.com is Stringhttp://www.google.com

The only thing lost if someone happens to get the types wrong in a way this could have caught is that they run it one extra time, get the failure, and fix the test. It's true that we could go ahead and give them both a half-helpful compile-time check, and better error messages. But I still am not convinced this complexity is paying for itself, especially when I look at CollectionSubject, et al.

Reply to this email directly or view it on GitHub:
#17 (comment)

Reply to this email directly or view it on GitHub:
#17 (comment)

Reply to this email directly or view it on GitHub:
#17 (comment)

@dsaff
Copy link
Owner Author

dsaff commented Aug 15, 2011

So you now believe ASSERT.that(myServer.getUrl()).is("http://www.google.com") should compile, or that we should introduce changes so that it won't?

@cgruber
Copy link
Contributor

cgruber commented Aug 16, 2011

I'm not sure how we can make it not compile. If you want to get to
"object" as the type compared, then this should compile. Unless we
decide that is() shouldn't exist for the default object, to avoid such
bad comparisons. I could go either way (for DefaultSubject) as long
as we make the failure more clear to the reader.

Regards,
Christian
Sent from my iPhone.

On Aug 15, 2011, at 13:27, dsaff
[email protected]
wrote:

So you now believe ASSERT.that(myServer.getUrl()).is("http://www.google.com") should compile, or that we should introduce changes so that it won't?

Reply to this email directly or view it on GitHub:
#17 (comment)

@dsaff
Copy link
Owner Author

dsaff commented Aug 17, 2011

I agree that our output can be better, and that we should look for
ways to do that. That's somewhat orthogonal, however, to what we want
to compile

So there are many places we could go with this:

  1. This is the current situation:
  • Subject.is(T other)
  • DefautSubject TestVerb.that(Object)

Consequences:

  • Compiles: yes: ASSERT.that(someUrl).is(someString)
  • Compiles: no: ASSERT.that(someString).is(someUrl)
  1. This is a proposal that I raised, and I thought at one point you
    preferred, but now I'm not sure:
  • Subject.is(T other)
  • DefautSubject TestVerb.that(T)

Consequences:

  • Compiles: no: ASSERT.that(someUrl).is(someString)
  • Compiles: no: ASSERT.that(someString).is(someUrl)
  1. This is my favorite proposal:
  • Subject.is(Object other)
  • DefautSubject TestVerb.that(Object)

Consequences:

  • Compiles: yes: ASSERT.that(someUrl).is(someString)
  • Compiles: yes: ASSERT.that(someString).is(someUrl)

Why do I prefer #3?

  • In general, generic types have proven such a pain in frameworks like
    hamcrest that I kind of want the experience of making the opposite
    mistake, that of using generics too little, rather than too much.
  • In this case, the T for Subject.is(T) needs to get passed around
    everywhere, for a method that makes up a small percentage of the API.
    Even if I like generics, I'm not sure about this trade-off.
  • Even more strongly, Java doesn't force type safety here, meaning
    that we're not getting support from the language. If
    someString.equals(someUrl) compiles, I think the principle of least
    surprise says that ASSERT.that(someString).is(someUrl) should compile,
    too.

What's your preference, and why?

David Saff

On Mon, Aug 15, 2011 at 8:51 PM, cgruber
[email protected]
wrote:

I'm not sure how we can make it not compile.  If you want to get to
"object" as the type compared, then this should compile.  Unless we
decide that is() shouldn't exist for the default object, to avoid such
bad comparisons.  I could go either way (for DefaultSubject) as long
as we make the failure more clear to the reader.

Regards,
Christian
Sent from my iPhone.

On Aug 15, 2011, at 13:27, dsaff
[email protected]
wrote:

So you now believe ASSERT.that(myServer.getUrl()).is("http://www.google.com") should compile, or that we should introduce changes so that it won't?

Reply to this email directly or view it on GitHub:
#17 (comment)

Reply to this email directly or view it on GitHub:
#17 (comment)

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

2 participants