jOOQ / jOOL

jOOλ - The Missing Parts in Java 8 jOOλ improves the JDK libraries in areas where the Expert Group's focus was elsewhere. It adds tuple support, function support, and a lot of additional functionality around sequential Streams. The JDK 8's main efforts (default methods, lambdas, and the Stream API) were focused around maintaining backwards compatibility and implementing a functional API for parallelism.
http://www.jooq.org/products
Apache License 2.0
2.08k stars 167 forks source link

Some methods on Seq are not correctly implemented #130

Closed sargue closed 8 years ago

sargue commented 8 years ago

I was debugging some parallel streams code that uses Seq and found this code inside Seq.

    // These methods have no effect
    // ----------------------------

    @Override
    default Seq<T> sequential() {
        return this;
    }

    @Override
    default Seq<T> parallel() {
        return this;
    }

Well, that's... unexpected. That also applies to some other methods like unordered().

So may we consider a bug not documenting something THAT important. IMHO. Javadocs at least.

Also SeqImpl has this code:

    @Override
    public boolean isParallel() {
        return false;
    }

So there is clearly a design decision. By the way, @lukaseder, could you please explain why?

lukaseder commented 8 years ago

The idea behind jOOQ is:

It adds tuple support, function support, and a lot of additional functionality around sequential Streams

Seq is short for sequential. It's the whole point of the type. E.g. the Javadoc:

A sequential, ordered {@link Stream} that adds all sorts of useful methods that work only because it is sequential and ordered.

How else / better could we document this? :)

lukaseder commented 8 years ago

... we could of course discuss throwing UnsupportedOperationException instead...

sargue commented 8 years ago

LOL, sorry about that. I always thought of Seq as short of sequence. Like Stream that says what it is, not how it is traversed. Anyway, my fault.

Throwing UnsupportedOperationException is the fail fast option but maybe too overkill. If the Seq is passed to some other library who calls parallel() I rather prefer to it to work sequentially than to fail. Or you can also just pass the internal Stream.

Maybe just some warning in the javadocs of the overriden method. That's the first place I looked at.

sargue commented 8 years ago

I would glady send a PR but I'm not sure if you use some code generator like with jOOQ. If it is just plain source code I can write it (not so difficult to put some javadocs anyway, just willing to help).

lukaseder commented 8 years ago

LOL, sorry about that. I always thought of Seq as short of sequence. Like Stream that says what it is, not how it is traversed. Anyway, my fault.

Yeah, it's a silly name. I mean, we don't want to type SequentialStream as a class name all the time... :) Also, given that Scala also has a Seq type, which is parallelizable.

I've thought about this again. I also don't want these calls to fail. A Javadoc hint on all relevant methods is certainly a good idea.

We do use a source code generator (e.g. for generating Tuple0 - Tuple16, but not for Seq). So, feel free to send a PR, thank you very much!

sargue commented 8 years ago

Done.

lukaseder commented 8 years ago

The relevant changes discussed here are implemented in https://github.com/jOOQ/jOOL/pull/131

Arpit0492 commented 8 years ago

Yo ! tuples are needed.

lukaseder commented 8 years ago

Thank you for your feedback, @Arpit0492. What were you trying to say by "tuples are needed" in the context of this discussion?