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

Add support for longintmap #14

Merged
merged 7 commits into from
Oct 16, 2023
Merged

Conversation

sarveswaran-m
Copy link
Contributor

  • Adds support for LongIntMap

@sarveswaran-m sarveswaran-m added the enhancement New feature or request label Oct 12, 2023
@mchernyakov
Copy link
Contributor

@sarveswaran-m please add tests :)

@sarveswaran-m sarveswaran-m force-pushed the add-support-for-longintmap branch from 10c0452 to 3f1f844 Compare October 13, 2023 13:43
@sarveswaran-m
Copy link
Contributor Author

@mchernyakov Have added tests. Pls have a look

@mchernyakov
Copy link
Contributor

somehow we missed that there is already a PR about longint map #6

build.gradle Outdated
@@ -50,8 +55,8 @@ mavenPublishing {
}
}

sourceCompatibility = 11
targetCompatibility = 11
sourceCompatibility = 17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we should add in the readme that the lib uses java 17

Copy link
Contributor Author

@sarveswaran-m sarveswaran-m Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • On further thought, it is better not to bump up source/target compatibility, since no jdk 17 specific feature is used in the lib. It would only prevent applications written in jdk versions < 17 from using the lib. Reverted this

* test cases have to be repeated for each typed map interface.
*/
public abstract class AbstractMapTest {
protected final Random random = ThreadLocalRandom.current();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do that way, you use Random related to a thread that instantiates a class.
I would rather use ThreadLocalRandom.current() in a method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Good point. Made ThreadLocalRandom.current() to be invoked at method level.

Comment on lines 13 to 17
private LongIntMap map;
// Keep the default value to easily verify that this value is returned.
protected int defaultValue;
// Some methods return the default value of the underlying Fastutil implementation.
private static final int FASTUTIL_DEFAULT_VALUE = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a const should go on top

Suggested change
private LongIntMap map;
// Keep the default value to easily verify that this value is returned.
protected int defaultValue;
// Some methods return the default value of the underlying Fastutil implementation.
private static final int FASTUTIL_DEFAULT_VALUE = 0;
// Some methods return the default value of the underlying Fastutil implementation.
private static final int FASTUTIL_DEFAULT_VALUE = 0;
private LongIntMap map;
// Keep the default value to easily verify that this value is returned.
protected int defaultValue;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Done

@sarveswaran-m
Copy link
Contributor Author

somehow we missed that there is already a PR about longint map #6

Noticed it a bit too late :(

@mchernyakov
Copy link
Contributor

somehow we missed that there is already a PR about longint map #6

Noticed it a bit too late :(

ok, then you should close the old PR.

Also, do not forget to update the version if you plan to release.

Copy link
Contributor

@erdoganf erdoganf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM but project version needs to be updated to prepare for the next release. Also version update in the README as well.

@sarveswaran-m sarveswaran-m merged commit 987fce3 into master Oct 16, 2023
2 checks passed
@sarveswaran-m sarveswaran-m deleted the add-support-for-longintmap branch October 16, 2023 15:55
@mchernyakov
Copy link
Contributor

@sarveswaran-m @erdoganf the version should be 0.3.0!
The minor one corresponds to quick fixes. see https://semver.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants