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

Moe Sync #575

Merged
merged 4 commits into from Jun 12, 2019

Conversation

Projects
None yet
4 participants
@ronshapiro
Copy link
Contributor

commented Jun 12, 2019

This code has been reviewed and submitted internally. Feel free to discuss on the PR and we can submit follow-up changes as necessary.

Commits:

Update docs for upcoming removal of type parameter from IterableOfProtosSubject.

9260675


Update comparison, especially about failure messages, and other random things.

a9fbb70


Somewhat arbitrarily shuffle the order of the nav bar.

Thoughts:

  • Now that the main page addresses the comparison to AssertJ (and links to the more detailed comparison), I figure that's less important.
  • FAQ seems like a good thing to put first after Home.
  • Known Types is more of a reference, and the Javadoc is perhaps a better reference.

06287e1


Point links to truth.dev.

44257d2

cpovirk added some commits Jun 7, 2019

Update docs for upcoming removal of type parameter from IterableOfPro…
…tosSubject.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=252055248
Update comparison, especially about failure messages, and other rando…
…m things.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=252693716
Somewhat arbitrarily shuffle the order of the nav bar.
Thoughts:
- Now that the main page addresses the comparison to AssertJ (and links to the more detailed comparison), I figure that's less important.
- FAQ seems like a good thing to put first after Home.
- Known Types is more of a reference, and the Javadoc is perhaps a better reference.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=252696923
Point links to truth.dev.
RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=252699810

@googlebot googlebot added the cla: yes label Jun 12, 2019

@cpovirk cpovirk merged commit b4504a3 into gh-pages Jun 12, 2019

1 check passed

cla/google All necessary CLAs are signed

@cpovirk cpovirk deleted the sync-gh-pages-2019/06/11 branch Jun 12, 2019


Under Truth, `assertThat(listOfStrings).doesNotContain(integer)` passes, even
though your test is probably buggy. Under AssertJ, it doesn't compile.
though your test is probably buggy. Under AssertJ, it doesn't compile. We plan
to add static analysis to [Error Prone] to catch such bugs.

This comment has been minimized.

Copy link
@jbduncan

jbduncan Jun 12, 2019

@cpovirk I'm curious to know why this something you need static analysis for rather than having the compiler check it for you. Would you mind explaining the rationale behind this? :)

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

It's a similar argument to https://errorprone.info/bugpattern/CollectionIncompatibleType

Plus, when type inference fails on a call that should be permitted, the error messages are ugly:

javatests/com/google/common/collect/SetsTest.java:779:
containsExactly(java.util.List<java.lang.Object&java.io.Serializable&java.lang.Comparable<?
extends java.lang.Object&java.io.Serializable&java.lang.Comparable<?>>>...)
in com.google.common.truth.IterableSubject<capture#106 of ?
extends com.google.common.truth.CollectionSubject<?,java.util.List<java.lang.Object&java.io.Serializable&java.lang.Comparable<?
extends java.lang.Object&java.io.Serializable&java.lang.Comparable<?>>>,java.util.Collection<java.util.List<java.lang.Object&java.io.Serializable&java.lang.Comparable<?
extends java.lang.Object&java.io.Serializable&java.lang.Comparable<?>>>>>,java.util.List<java.lang.Object&java.io.Serializable&java.lang.Comparable<?
extends java.lang.Object&java.io.Serializable&java.lang.Comparable<?>>>,java.util.Collection<java.util.List<java.lang.Object&java.io.Serializable&java.lang.Comparable<?
extends java.lang.Object&java.io.Serializable&java.lang.Comparable<?>>>>>
cannot be applied to
(java.util.List<java.lang.Object>,java.util.List<java.lang.Object>,java.util.List<java.lang.Object>,java.util.List<java.lang.Object>)
    assertThat(cartesianProduct(x, y)).containsExactly(exp1,
exp2, exp3, exp4);

I'm still not 100% convinced that we made the right choice, but that's the theory. I'll note that in the docs. Thanks.

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

Hmm, actually, that's an old message from the days in which Subject and its subclasses had type parameters. Let me see what it would look like today.

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

It looks like probably even from an older javac, too.

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

(Back in the day, we didn't have @SafeVarargs, either, but that's now a non-issue.)

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

And interestingly, that particular error might not happen with the current javac.

So, while the logic of CollectionIncompatibleType still applies, E... has certainly gotten more attractive from all the other changes.

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

No, wait, I messed up my change to IterableSubject. Everything I said above is wrong :)

This comment has been minimized.

Copy link
@cpovirk

cpovirk Jun 13, 2019

Member

OK, trying again: Truth-that-requires-T would still fail on the cartesianProduct example, even under a new javac. The error is now:

core/src/test/java/com/google/common/truth/IterableSubjectTest.java:56: error: method containsExactly in class IterableSubject<E> cannot be applied to given types;
      .containsExactly(exp1, exp2, exp3, exp4);
      ^
  required: List<INT#1>[]
  found:    List<Object>,List<Object>,List<Object>,List<Object>
  reason: varargs mismatch; List<Object> cannot be converted to List<INT#1>
  where E is a type-variable:
    E extends Object declared in class IterableSubject
  where INT#1,INT#2 are intersection types:
    INT#1 extends Object,Serializable,Comparable<? extends INT#2>
    INT#2 extends Object,Serializable,Comparable<?>

And, as a digression here into something else relevant to my interests, the error message is just slightly longer if IterableSubject has self-type and actual-type parameters:

core/src/test/java/com/google/common/truth/IterableSubjectTest.java:56: error: method containsExactly in class IterableSubject<S,A,E> cannot be applied to given types;
      .containsExactly(exp1, exp2, exp3, exp4);
      ^
  required: List<INT#1>[]
  found:    List<Object>,List<Object>,List<Object>,List<Object>
  reason: varargs mismatch; List<Object> cannot be converted to List<INT#1>
  where S,A,E are type-variables:
    S extends IterableSubject<S,A,E> declared in class IterableSubject
    A extends Iterable<E> declared in class IterableSubject
    E extends Object declared in class IterableSubject
  where INT#1,INT#2 are intersection types:
    INT#1 extends Object,Serializable,Comparable<? extends INT#2>
    INT#2 extends Object,Serializable,Comparable<?>

This comment has been minimized.

Copy link
@jbduncan

jbduncan Jun 13, 2019

It's a similar argument to https://errorprone.info/bugpattern/CollectionIncompatibleType

Ahh, okay. I'm reasonably happy to accept that, then. :)

ronshapiro added a commit that referenced this pull request Jun 17, 2019

Explain why Truth's IterableSubject has looser types, for better or f…
…or worse.

#575 (comment)

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=253032775

@ronshapiro ronshapiro referenced this pull request Jun 17, 2019

Merged

Moe Sync #580

ronshapiro added a commit that referenced this pull request Jun 18, 2019

Explain why Truth's IterableSubject has looser types, for better or f…
…or worse.

#575 (comment)

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=253032775
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.