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

fest-481 : Assert.isIn() and isNotIn() should take Iterable<?> #15

Merged
merged 3 commits into from
May 5, 2012
Merged

Conversation

nfrancois
Copy link
Contributor

@ash2k
Copy link
Contributor

ash2k commented Apr 28, 2012

Great! Idea - why not make those methods even more type-safe (like isIn(A... values)) by using Iterable<? extends A>?
Actually, it seems like alot of places can be type-safer in this great library. It will give us compile time checks (instead of failing test at runtime because we passed wrong iterable instance) and more correct IDE autocompletion.

@nfrancois
Copy link
Contributor Author

@ash2k could be interesting.
Seem working on my computer, but before pushing, I prefer have a idea about on what can be more be easily writed with.
Did you got a example on what is painfull to write the Iterable<?> and become easy with Iterable<? extends A> ?

@joel-costigliola
Copy link
Contributor

@ash2k I agree with Nicolas, it would be nice to have a concrete example (that we could use for unit testing).

Thanks !

@ash2k
Copy link
Contributor

ash2k commented Apr 29, 2012

This can't be used as a unit test but it shows benefits of generics:

public class Test1 {

    private final A var1 = new A();
    private final B var2 = new B();

    public void a() {
        Collection<B> colB = codeUnderTest();

        // wanted to write
        // Assertions.assertThat(var2).isNotIn(colB);
        Assertions.assertThat(var1).isNotIn(colB);
    }

    public Collection<B> codeUnderTest() {
        // broken logic. should be
        // return new ArrayList<B>(Arrays.asList(new B()));
        return new ArrayList<B>(Arrays.asList(var2));
    }

    public class A {
    }

    public class B {
    }
}

With current implementation (wildcards instead of captured types) this broken test will compile and pass not detecting the error in codeUnderTest(). If ObjectAssert is modified to be parametrized with type A the test will not compile and Eclipse is showing error

The method isNotIn(Test1.A...) in the type AbstractAssert<ObjectAssert<Test1.A>,Test1.A> is not applicable for the arguments (Collection<Test1.B>)

Java is a staticly typed language and generics were added to provide means to do more strict compile-time type checking. I think the question is "Why not use them?" than "Why use them?".

What do you think?

@nfrancois
Copy link
Contributor Author

@ash2k thanks your example. It's true check if a element is/is not in different type list, it does not make sense.

I try to write :

assertThat(frodo).isNotIn(ringsOfPower);

Should not compile, but it compile just because type of A is Object.
I think I must be ok with #11

@joel-costigliola
Copy link
Contributor

I think we should go as Mikhail suggest.

@joel-costigliola
Copy link
Contributor

Nicolas,
I'm would like to release Fest Assert 2.0M3 this week end, it would be nice to include fest-481 fix.
Will you have time to finish it with Mikhail suggestion ?
If you don't, there is no problem, I will do it.

Cheers,

Joel

@nfrancois
Copy link
Contributor Author

I have already pushed this suggestion in this commit :

https://github.com/nfrancois/fest-assert-2.x/commit/18ac8197505121ad8962738ebb7f9bf973a1ac7a

Could be some conflict with #11, I let you see

@joel-costigliola
Copy link
Contributor

Ok, my bad, did not see your commit.

Thanks !

@joel-costigliola joel-costigliola merged commit 18ac819 into alexruiz:master May 5, 2012
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

Successfully merging this pull request may close these issues.

3 participants