fastutil icon indicating copy to clipboard operation
fastutil copied to clipboard

[Feature request] Contributor doc

Open Arthur-Milchior opened this issue 3 years ago • 10 comments

Hello

Thanks a lot for this library. I've got a few remarks on the documentation, I believe they may help people discovering the library you'd accept to consider it. In your readme, the link mailto:[email protected] is actually broken, in the sens that if you write to it, you get a message stating that you can't post unless you're a member of the group. So I guess that a link to the group would be more useful.

Is there rules for contributors. As you may have seen, I may be interested in contributing features I want to use, but I don't know what are you rules regarding the expected amount of tests. I would be willing to contribute a small file explaining my understanding of DRV and what I needed to figure out to consider creating a Pair class, so that other contributors don't have to find out how this work. But I should admit I would not feel confident that I got things right

Arthur-Milchior avatar Oct 28 '20 16:10 Arthur-Milchior

Yeah, I know. People who contributed have guessed their way through gencsources.sh, which is of course suboptimal. For the first 20 years or so actually nobody was contributing, so frankly I didn't see the need. But there are more and more people willing to do that.

There are type-specific Pair classes in Eclipse Collections (try searching for IntIntPair), but of course it wouldn't make much sense to use those, and also I don't like getOne()/getTwo(). I would use something like first()/second().

I think the Eclipse idea of having a Pair interface is good. We would then have a BasicPair implementation with the two obvious fields. Or maybe a MutablePair and an ImmutablePair implementations—they might come handy. What do you think?

vigna avatar Oct 28 '20 17:10 vigna

Do you prefer we get all discussion in a single issue ? I separated them to try to keep discussion simpler.

I like the idea of having Mutable and Immutable pair. I didn't thought about it, but it makes a lot of sens.

I tried to create immutable pair in https://github.com/Arthur-Milchior/fastutil/tree/pair but this ucrrently does not work. For an unknown reason, the second value is always V instead of being specialized. E.g. in IntDoublePair.c , there is #define VALUES_REFERENCE 1 while this is clearly a primitive value

Arthur-Milchior avatar Oct 28 '20 18:10 Arthur-Milchior

Problem: contributing to fastutil is done releasing the copyright to me (it has been always this way). That has been a problem with another large pull request. Can you do that?

vigna avatar Oct 28 '20 18:10 vigna

I can ask my employer. I have no idea what their answer would be and it usually takes a few day to get the answer.

If you can help me debug the problem, that would be appreciated nonetheless. If it matters, I intend to use this library for https://github.com/ankidroid/Anki-Android and not for any work related purpose.

Arthur-Milchior avatar Oct 28 '20 18:10 Arthur-Milchior

I just send the permission request

Arthur-Milchior avatar Oct 28 '20 18:10 Arthur-Milchior

We would then have a BasicPair implementation with the two obvious fields. Or maybe a MutablePair and an ImmutablePair implementations—they might come handy. What do you think?

I think for such value classes immutability is better. They are much easier to optimize for the compiler and clearer to use. However, the performance impact should probably be tested.

I'd also suggest factory methods in the interface + hiding the "BasicPair" implementation. Something like Pair.of(a, b) or Pair.mutable(a, b).

I don't like getOne()/getTwo(). I would use something like first()/second().

I also have seen left() and right().

incaseoftrouble avatar Oct 28 '20 19:10 incaseoftrouble

Oh yeah, left()/right() is even better.

vigna avatar Oct 28 '20 19:10 vigna

Problem: contributing to fastutil is done releasing the copyright to me (it has been always this way). That has been a problem with another large pull request. Can you do that?

By the way, since this feature request is about "Contributor doc"; that's also some information that would have been nice to have in a contributor doc, if possible

Arthur-Milchior avatar Oct 28 '20 19:10 Arthur-Milchior

Also useful info for the contributor doc: How to build and test it? Just saying ant jar and ant javadoc isn't enough because that doesn't test anything, and ant junit doesn't work out-of-the-box.

magneticflux- avatar Nov 24 '20 00:11 magneticflux-

That should be fixed with 87be2b3030.

vigna avatar Nov 24 '20 09:11 vigna