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

[Feature request] Contributor doc #175

Open
Arthur-Milchior opened this issue Oct 28, 2020 · 10 comments
Open

[Feature request] Contributor doc #175

Arthur-Milchior opened this issue Oct 28, 2020 · 10 comments

Comments

@Arthur-Milchior
Copy link

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:fastutil@googlegroups.com 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

@vigna
Copy link
Owner

vigna commented Oct 28, 2020

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?

@Arthur-Milchior
Copy link
Author

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

@vigna
Copy link
Owner

vigna commented Oct 28, 2020

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?

@Arthur-Milchior
Copy link
Author

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
Copy link
Author

I just send the permission request

@incaseoftrouble
Copy link
Collaborator

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().

@vigna
Copy link
Owner

vigna commented Oct 28, 2020

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

@Arthur-Milchior
Copy link
Author

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

@magneticflux-
Copy link

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.

@vigna
Copy link
Owner

vigna commented Nov 24, 2020

That should be fixed with 87be2b3.

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

No branches or pull requests

4 participants