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

Should Object-based methods be in the base Subject? #20

Open
dsaff opened this issue Jun 9, 2011 · 17 comments
Open

Should Object-based methods be in the base Subject? #20

dsaff opened this issue Jun 9, 2011 · 17 comments

Comments

@dsaff
Copy link
Owner

dsaff commented Jun 9, 2011

Currently, the Subject base class contains Object-based methods: is, isEqualTo, isInstanceOf, etc...

I think there's an advantage to pushing those methods down to an ObjectSubject class. For example, there could be subjects for which isEqualTo isn't defined/useful. We may choose to do this with IntSubject: ASSERT.that(x).is(373) is of questionable utility.
To leave this option open, and due to SRP, I think this might be a good idea. DefaultSubject may not need to be anything better than ObjectSubject, in this situation.

@cgruber
Copy link
Contributor

cgruber commented Jun 10, 2011

We'll have problems with chaining if we do that, since we'll have something match Object to get isInstanceOf and then can't do any non-Object work.

Now is, isEqualTo may be reasonable, so the custom types need to do their own, but instanceof (isA()) is appropriate for the base, I think, since they all need it.

Let's draw up a table of things that are truly universal (should be on Subject) and things that are specific, but for which we need Object implementations for default behavour (isEqualTo()).

@dsaff
Copy link
Owner Author

dsaff commented Jun 13, 2011

Can you give an example of a chain that works now, but wouldn't in my suggested world? I think I've swapped an important point out of memory. Thanks.

@cgruber
Copy link
Contributor

cgruber commented Jun 13, 2011

Wouldn't ASSERT.that("some string").isA(String.class).and().contains("some");

The Subject returned from isA() would be DefaultSubject (or ObjectSubject or whatever), which doesn't contain any of StringSubject's contract.

Christian.

On Jun 13, 2011, at 11:01 AM, dsaff wrote:

Can you give an example of a chain that works now, but wouldn't in my suggested world? I think I've swapped an important point out of memory. Thanks.

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360657

@cgruber
Copy link
Contributor

cgruber commented Jun 13, 2011

Actually, it wouldn't compile, and furthermore, you couldn't get isA() on ASSERT.that("some string"). That would return StringSubject, which would not have the isA() contract in this universe.

@dsaff
Copy link
Owner Author

dsaff commented Jun 13, 2011

Oh, I was assuming StringSubject would inherit from ObjectSubject...

On Mon, Jun 13, 2011 at 2:16 PM, cgruber
reply@reply.github.com
wrote:

Actually, it wouldn't compile, and furthermore, you couldn't get isA() on ASSERT.that("some string").  That would return StringSubject, which would not have the isA() contract in this universe.

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360759

@cgruber
Copy link
Contributor

cgruber commented Jun 13, 2011

Dude... "Subject" is "ObjectSubject". That's how it works now. I just have "DefaultSubject so that things that don't match have a concrete thing to match on. It's a no-op sub-class of Subject. But Subject does contain all the isA() stuff.

On Jun 13, 2011, at 11:36 AM, dsaff wrote:

Oh, I was assuming StringSubject would inherit from ObjectSubject...

On Mon, Jun 13, 2011 at 2:16 PM, cgruber
reply@reply.github.com
wrote:

Actually, it wouldn't compile, and furthermore, you couldn't get isA() on ASSERT.that("some string"). That would return StringSubject, which would not have the isA() contract in this universe.

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360759

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360911

@dsaff
Copy link
Owner Author

dsaff commented Jun 14, 2011

OK, I think names are beginning to get in our way here. I'm imagining:

current world:

Subject { nextChain, fail, isA, is, ... }
DefaultSubject extends Subject {}
StringSubject extends Subject { contains, startsWith, ... }
IntegerSubject extends Subject { isBetween, @OverRide isEqualTo, ... }

new, proposed world:

Subject { nextChain, fail, ... }
ObjectSubject extends Subject { isA, is, ... }
StringSubject extends ObjectSubject { contains, startsWith, ... }
IntegerSubject extends Subject { isBetween, isEqualTo }

Does this help?

On Mon, Jun 13, 2011 at 2:39 PM, cgruber
reply@reply.github.com
wrote:

Dude... "Subject" is "ObjectSubject".  That's how it works now.  I just have "DefaultSubject so that things that don't match have a concrete thing to match on.  It's a no-op sub-class of Subject.  But Subject does contain all the isA() stuff.

On Jun 13, 2011, at 11:36 AM, dsaff wrote:

Oh, I was assuming StringSubject would inherit from ObjectSubject...

On Mon, Jun 13, 2011 at 2:16 PM, cgruber
reply@reply.github.com
wrote:

Actually, it wouldn't compile, and furthermore, you couldn't get isA() on ASSERT.that("some string").  That would return StringSubject, which would not have the isA() contract in this universe.

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360759

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360911

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360934

@cgruber
Copy link
Contributor

cgruber commented Jun 14, 2011

Ahhhhh... sorry I see it now. You're adding a layer between Subject and StringSubject which would NOT be a parent to IntegerSubject?

The problem is that isA is applicable to primitive types, etc. I think there's less contract delta between Subject and ObjectSubject than you might think. I can see isA() and is() being applicable to primitives... what other contract would you imagine being different?

Christian

On Jun 14, 2011, at 5:36 AM, dsaff wrote:

OK, I think names are beginning to get in our way here. I'm imagining:

current world:

Subject { nextChain, fail, isA, is, ... }
DefaultSubject extends Subject {}
StringSubject extends Subject { contains, startsWith, ... }
IntegerSubject extends Subject { isBetween, @OverRide isEqualTo, ... }

new, proposed world:

Subject { nextChain, fail, ... }
ObjectSubject extends Subject { isA, is, ... }
StringSubject extends ObjectSubject { contains, startsWith, ... }
IntegerSubject extends Subject { isBetween, isEqualTo }

Does this help?

On Mon, Jun 13, 2011 at 2:39 PM, cgruber
reply@reply.github.com
wrote:

Dude... "Subject" is "ObjectSubject". That's how it works now. I just have "DefaultSubject so that things that don't match have a concrete thing to match on. It's a no-op sub-class of Subject. But Subject does contain all the isA() stuff.

On Jun 13, 2011, at 11:36 AM, dsaff wrote:

Oh, I was assuming StringSubject would inherit from ObjectSubject...

On Mon, Jun 13, 2011 at 2:16 PM, cgruber
reply@reply.github.com
wrote:

Actually, it wouldn't compile, and furthermore, you couldn't get isA() on ASSERT.that("some string"). That would return StringSubject, which would not have the isA() contract in this universe.

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360759

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360911

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360934

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1365638

@dsaff
Copy link
Owner Author

dsaff commented Jun 14, 2011

What is a situation where someone would really want to
ASSERT.that(3.0).isA(double.class)?

A big thing I want to just remove from being possible is

ASSERT.that(3.00000001).isEqualTo(3.000000002);

David Saff

On Tue, Jun 14, 2011 at 12:22 PM, cgruber
reply@reply.github.com
wrote:

Ahhhhh... sorry I see it now.  You're adding a layer between Subject and StringSubject which would NOT be a parent to IntegerSubject?

The problem is that isA is applicable to primitive types, etc.  I think there's less contract delta between Subject and ObjectSubject than you might think.  I can see isA() and is() being applicable to primitives... what other contract would you imagine being different?

Christian

On Jun 14, 2011, at 5:36 AM, dsaff wrote:

OK, I think names are beginning to get in our way here.  I'm imagining:

current world:

Subject { nextChain, fail, isA, is, ... }
DefaultSubject extends Subject {}
StringSubject extends Subject { contains, startsWith, ... }
IntegerSubject extends Subject { isBetween, @OverRide isEqualTo, ... }

new, proposed world:

Subject { nextChain, fail, ... }
ObjectSubject extends Subject { isA, is, ... }
StringSubject extends ObjectSubject { contains, startsWith, ... }
IntegerSubject extends Subject { isBetween, isEqualTo }

Does this help?

On Mon, Jun 13, 2011 at 2:39 PM, cgruber
reply@reply.github.com
wrote:

Dude... "Subject" is "ObjectSubject".  That's how it works now.  I just have "DefaultSubject so that things that don't match have a concrete thing to match on.  It's a no-op sub-class of Subject.  But Subject does contain all the isA() stuff.

On Jun 13, 2011, at 11:36 AM, dsaff wrote:

Oh, I was assuming StringSubject would inherit from ObjectSubject...

On Mon, Jun 13, 2011 at 2:16 PM, cgruber
reply@reply.github.com
wrote:

Actually, it wouldn't compile, and furthermore, you couldn't get isA() on ASSERT.that("some string").  That would return StringSubject, which would not have the isA() contract in this universe.

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360759

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360911

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1360934

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1365638

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367175

@cgruber
Copy link
Contributor

cgruber commented Jun 14, 2011

ASSERT.that(someObjectVariableWhichMayContainANumeric).isA(Double.class);

And why do you want to remove ASSERT.that(3.00000001).isEqualTo(3.000000002); from being possible?

On Jun 14, 2011, at 10:24 AM, dsaff wrote:

What is a situation where someone would really want to
ASSERT.that(3.0).isA(double.class)?

A big thing I want to just remove from being possible is

ASSERT.that(3.00000001).isEqualTo(3.000000002);

David Saff

@dsaff
Copy link
Owner Author

dsaff commented Jun 14, 2011

On Tue, Jun 14, 2011 at 1:33 PM, cgruber
reply@reply.github.com
wrote:

ASSERT.that(someObjectVariableWhichMayContainANumeric).isA(Double.class);

This will trigger ASSERT.that(Object), which will return ObjectSubject.

And why do you want to remove ASSERT.that(3.00000001).isEqualTo(3.000000002); from being possible?

Because people always

ASSERT.that(2.0 + 2.0).isEqualTo(4.0),

and get flustered when it fails. There must be a delta, or it isn't meaningful.

David

On Jun 14, 2011, at 10:24 AM, dsaff wrote:

What is a situation where someone would really want to
ASSERT.that(3.0).isA(double.class)?

A big thing I want to just remove from being possible is

ASSERT.that(3.00000001).isEqualTo(3.000000002);

  David Saff

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367725

@cgruber
Copy link
Contributor

cgruber commented Jun 14, 2011

So, I think doubles are a separate issue, and I think you're trying to use the contract heirarchy to deal with the fact that Java lets you do stupid things. Maybe that makes sense. But IntegerSubject can reasonably do .that(4).is(4). It's just floating point and other inexact numerics. I wonder if we should fix it by not allowing is() for that one, or if it's more sensible to have errors from the DoubleSubject which are more robust.

The other possibility is the separation you suggest, though I wouldn't make all primitives inherit from the root - rather, than ObjectSubject, make it EquatableSubject or something. Because I think booleans and Integers and just about everything except doubles/floats should be equatable.

Christian.

On Jun 14, 2011, at 10:46 AM, dsaff wrote:

On Tue, Jun 14, 2011 at 1:33 PM, cgruber
reply@reply.github.com
wrote:

ASSERT.that(someObjectVariableWhichMayContainANumeric).isA(Double.class);

This will trigger ASSERT.that(Object), which will return ObjectSubject.

And why do you want to remove ASSERT.that(3.00000001).isEqualTo(3.000000002); from being possible?

Because people always

ASSERT.that(2.0 + 2.0).isEqualTo(4.0),

and get flustered when it fails. There must be a delta, or it isn't meaningful.

David

On Jun 14, 2011, at 10:24 AM, dsaff wrote:

What is a situation where someone would really want to
ASSERT.that(3.0).isA(double.class)?

A big thing I want to just remove from being possible is

ASSERT.that(3.00000001).isEqualTo(3.000000002);

David Saff

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367725

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367815

@dsaff
Copy link
Owner Author

dsaff commented Jun 14, 2011

I'm not as worried about whether Java lets you do stupid things (it
does) than whether a user scanning the API will see a method that
seems to do a smart thing, and instead does something stupid.

So, for all primitive types xxx, I'd like to prevent (assuming "is" is
an alias, uninteresting to this conversation):

(xxx).isA
(xxx).isNotA
(xxx).isSameAs

I'd also like to prevent

(double).isEqualTo(double)
(double).isNotEqualTo(double)

Again, where xxx is any primitive, I'm not a big fan of

(xxx).isNull
(xxx).isNotNull

But I can cope with either decision.

Note that the proposed separation of Subject and ObjectSubject also
allows for custom subjects whose authors would like to hide
information about the wrapped objects, to encourage robust test
authoring. For example, I might want to enable:

ASSERT.with(USERS).that(currentUser()).isLoggedIn()

Without exposing the ability to say

ASSERT.with(USERS).that(currentUser()).isA(ImplementationSpecificInstanceOfSomeUserClass.class)

Thoughts?

David Saff
On Tue, Jun 14, 2011 at 1:54 PM, cgruber
reply@reply.github.com
wrote:

So, I think doubles are a separate issue, and I think you're trying to use the contract heirarchy to deal with the fact that Java lets you do stupid things. Maybe that makes sense.  But IntegerSubject can reasonably do .that(4).is(4).  It's just floating point and other inexact numerics.  I wonder if we should fix it by not allowing is() for that one, or if it's more sensible to have errors from the DoubleSubject which are more robust.

The other possibility is the separation you suggest, though I wouldn't make all primitives inherit from the root - rather, than ObjectSubject, make it EquatableSubject or something.  Because I think booleans and Integers and just about everything except doubles/floats should be equatable.

Christian.

On Jun 14, 2011, at 10:46 AM, dsaff wrote:

On Tue, Jun 14, 2011 at 1:33 PM, cgruber
reply@reply.github.com
wrote:

ASSERT.that(someObjectVariableWhichMayContainANumeric).isA(Double.class);

This will trigger ASSERT.that(Object), which will return ObjectSubject.

And why do you want to remove ASSERT.that(3.00000001).isEqualTo(3.000000002); from being possible?

Because people always

ASSERT.that(2.0 + 2.0).isEqualTo(4.0),

and get flustered when it fails.  There must be a delta, or it isn't meaningful.

  David

On Jun 14, 2011, at 10:24 AM, dsaff wrote:

What is a situation where someone would really want to
ASSERT.that(3.0).isA(double.class)?

A big thing I want to just remove from being possible is

ASSERT.that(3.00000001).isEqualTo(3.000000002);

  David Saff

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367725

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367815

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367872

@dsaff
Copy link
Owner Author

dsaff commented Jun 14, 2011

Arggh. Please s/[.]with/.about/ throughout.

David Saff

On Tue, Jun 14, 2011 at 2:41 PM, David Saff david@saff.net wrote:

I'm not as worried about whether Java lets you do stupid things (it
does) than whether a user scanning the API will see a method that
seems to do a smart thing, and instead does something stupid.

So, for all primitive types xxx, I'd like to prevent (assuming "is" is
an alias, uninteresting to this conversation):

(xxx).isA
(xxx).isNotA
(xxx).isSameAs

I'd also like to prevent

(double).isEqualTo(double)
(double).isNotEqualTo(double)

Again, where xxx is any primitive, I'm not a big fan of

(xxx).isNull
(xxx).isNotNull

But I can cope with either decision.

Note that the proposed separation of Subject and ObjectSubject also
allows for custom subjects whose authors would like to hide
information about the wrapped objects, to encourage robust test
authoring.  For example, I might want to enable:

ASSERT.with(USERS).that(currentUser()).isLoggedIn()

Without exposing the ability to say

ASSERT.with(USERS).that(currentUser()).isA(ImplementationSpecificInstanceOfSomeUserClass.class)

Thoughts?

  David Saff
On Tue, Jun 14, 2011 at 1:54 PM, cgruber
reply@reply.github.com
wrote:

So, I think doubles are a separate issue, and I think you're trying to use the contract heirarchy to deal with the fact that Java lets you do stupid things. Maybe that makes sense.  But IntegerSubject can reasonably do .that(4).is(4).  It's just floating point and other inexact numerics.  I wonder if we should fix it by not allowing is() for that one, or if it's more sensible to have errors from the DoubleSubject which are more robust.

The other possibility is the separation you suggest, though I wouldn't make all primitives inherit from the root - rather, than ObjectSubject, make it EquatableSubject or something.  Because I think booleans and Integers and just about everything except doubles/floats should be equatable.

Christian.

On Jun 14, 2011, at 10:46 AM, dsaff wrote:

On Tue, Jun 14, 2011 at 1:33 PM, cgruber
reply@reply.github.com
wrote:

ASSERT.that(someObjectVariableWhichMayContainANumeric).isA(Double.class);

This will trigger ASSERT.that(Object), which will return ObjectSubject.

And why do you want to remove ASSERT.that(3.00000001).isEqualTo(3.000000002); from being possible?

Because people always

ASSERT.that(2.0 + 2.0).isEqualTo(4.0),

and get flustered when it fails.  There must be a delta, or it isn't meaningful.

  David

On Jun 14, 2011, at 10:24 AM, dsaff wrote:

What is a situation where someone would really want to
ASSERT.that(3.0).isA(double.class)?

A big thing I want to just remove from being possible is

ASSERT.that(3.00000001).isEqualTo(3.000000002);

  David Saff

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367725

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367815

Reply to this email directly or view it on GitHub:
https://github.com/dsaff/truth/issues/20#issuecomment-1367872

@cgruber
Copy link
Contributor

cgruber commented Jul 5, 2011

Ok. I think, after re-reading this, I can jibe with this. I'd like to migrate over to truth0/truth first, so if we can get David's work in and do that, then we can move on this stuff.

@dsaff
Copy link
Owner Author

dsaff commented Jul 5, 2011

Sounds good. Am I blocking the truth0 move? Can I help?

@cgruber
Copy link
Contributor

cgruber commented Jul 5, 2011

Just pulling in hagbard's changes when they're ready. There's an outstanding pull request I'd rather have in because it fixes the build.

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