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.

Sign up for GitHub

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

Jump to bottom

Expose Pair and its factory method #9

Merged
merged 5 commits into from
Jan 2, 2020
Merged

Expose Pair and its factory method #9

merged 5 commits into from
Jan 2, 2020

Conversation

hisener
Copy link
Contributor

@hisener hisener commented Dec 31, 2019

No description provided.

@hisener hisener requested a review from Stephan202 December 31, 2019 15:08
hisener
hisener commented Dec 31, 2019
View reviewed changes
src/main/java/tech/picnic/jolo/Relation.java Outdated
@@ -26,7 +26,7 @@
* @param <L> The Java class that the left-hand side of the relation is mapped to
* @param <R> The Java class that the right-hand side of the relation is mapped to.
*/
final class Relation<L, R> {
public final class Relation<L, R> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because Pair is static nested class. We can move it to a separate file. IDK which one is more desirable.

Copy link
Member

Choose a reason for hiding this comment

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

If this file does not otherwise need to be public (didn't check) then yes I think moving it out makes sense. The smaller the API surface, the better.

@codecov-io
Copy link

codecov-io commented Dec 31, 2019 •
edited
Loading

Codecov Report

Merging #9 into master will increase coverage by 1.19%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #9      +/-   ##
============================================
+ Coverage     87.97%   89.16%   +1.19%     
  Complexity      117      117              
============================================
  Files             8        8              
  Lines           291      277      -14     
  Branches         26       23       -3     
============================================
- Hits            256      247       -9     
+ Misses            4        2       -2     
+ Partials         31       28       -3
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/tech/picnic/jolo/RelationBuilder.java 81.81% <100%> (ø) 28 <1> (ø) ⬇️
src/main/java/tech/picnic/jolo/Relation.java 92.06% <100%> (+5.05%) 26 <5> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65701e8...5fd3160. Read the comment docs.

Mordavolt
Mordavolt approved these changes Jan 1, 2020
View reviewed changes
Copy link

@Mordavolt Mordavolt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This LGTM, but I think codecov is a bit too overzealous.

@Stephan202
Copy link
Member

I guess we should review the Codecov settings, but OTOH that class should best tested. Not sure how the percentage is calculated exactly. If there are no other takers I'll probably spend some time later today/tomorrow to just add the relevant tests. (Since the class is now public I'd even argue we should add some documentation.)

@Stephan202
Copy link
Member

(Rebased.)

@Mordavolt
Copy link

I would rather add Lombok or Guava as a build-only dependency and generate .of(), .hashCode() and .equals() instead of writing them out and covering with tests.

@Stephan202
Copy link
Member

We can add Immutables as a build-only dependency; works for me 👍

@hisener hisener mentioned this pull request Jan 1, 2020
Merged
@Stephan202
Copy link
Member

Pushed a commit in which Pair is generated using Immutables. Ideally the pom.xml changes are ported to oss-parent, but that can be done at a later stage.

@Stephan202
Copy link
Member

Added some documentation. Given the names of the accessor methods (getLeftId() and getRightId()) I think IdPair could be a better name. WDYT?

@hisener
Copy link
Contributor Author

hisener commented Jan 1, 2020 •
edited
Loading

Changes LGTM.

I think IdPair could be a better name. WDYT?

Works for me.

Stephan202
Stephan202 approved these changes Jan 1, 2020
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Approving. Note to self: port the pom.xml changes to oss-parent.

@Mordavolt
Copy link

LGTM

@Stephan202
Copy link
Member

Suggested commit message:

Expose `Pair` as `IdPair` (#9)

The class and its factory method are now public.

@Stephan202 Stephan202 merged commit 31546cf into master Jan 2, 2020
@Stephan202 Stephan202 added this to the 0.0.2 milestone Jan 3, 2020
@Stephan202 Stephan202 deleted the hsener/expose-pair branch January 4, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mordavolt Mordavolt Mordavolt approved these changes

@Stephan202 Stephan202 Stephan202 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
0.0.2
Development

Successfully merging this pull request may close these issues.
4 participants
@hisener @codecov-io @Stephan202 @Mordavolt