search

broadinstitute / wdl4s

Scala bindings for WDL
3 stars 5 forks source link

Switching GraphNode equality to object identity-based #249

Closed curoli closed 7 years ago

curoli commented 7 years ago

Switched GraphNode.equals and GraphNode.hashCode to object identity-based equality instead of component-based.

Updated two unit tests that relied on conponent-based equality.

danbills commented 7 years ago

I'd rather not do this, as equals is supposed to be bitwise/value-based equality, whereas eq indicates reference equality.

mcovarr commented 7 years ago

Where was equals and hashCode being called that motivated these changes? I share @danbills's concerns about changing these to mean reference equality, it may make more sense to change the way we're using GraphNodes.

cjllanwarne commented 7 years ago

👍

Approved with PullApprove

Horneth commented 7 years ago

Should this PR be moved to Cromwell now that we merged wdl4s into the cromwell repo ?

curoli commented 7 years ago

I guess it should. How do I do that?

On Wed, Oct 11, 2017 at 10:34 AM, Thib notifications@github.com wrote:

Should this PR be moved to Cromwell now that we merged wdl4s into the cromwell repo ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/broadinstitute/wdl4s/pull/249#issuecomment-335831466, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_4aP2jdz_5l56fsGN2vqrQ-bOfpRxgks5srNIMgaJpZM4Pt3Cd .

-- Oliver Ruebenacker Senior Software Engineer, Diabetes Portal http://www.type2diabetesgenetics.org/, Broad Institute http://www.broadinstitute.org/

Horneth commented 7 years ago

You could do that https://stackoverflow.com/a/5120074/1498572

Horneth commented 7 years ago

@curoli Just curious about the status on this, I actually would benefit from it in a branch I have :)

curoli commented 7 years ago

Working on it. Hope to finish today.

On Thu, Oct 12, 2017 at 10:46 AM, Thib notifications@github.com wrote:

@curoli https://github.com/curoli Just curious about the status on this, I actually would benefit from it in a branch I have :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/broadinstitute/wdl4s/pull/249#issuecomment-336160163, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_4aBAv1jNtzaaDNObsIKyct3v9pXxwks5sriZPgaJpZM4Pt3Cd .

-- Oliver Ruebenacker Senior Software Engineer, Diabetes Portal http://www.type2diabetesgenetics.org/, Broad Institute http://www.broadinstitute.org/

curoli commented 7 years ago

Submitted PR. Planning to merge once Travis builds pass.

On Thu, Oct 12, 2017 at 1:15 PM, Oliver Ruebenacker < oliverr@broadinstitute.org> wrote:

Working on it. Hope to finish today.

On Thu, Oct 12, 2017 at 10:46 AM, Thib notifications@github.com wrote:

@curoli https://github.com/curoli Just curious about the status on this, I actually would benefit from it in a branch I have :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/broadinstitute/wdl4s/pull/249#issuecomment-336160163, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_4aBAv1jNtzaaDNObsIKyct3v9pXxwks5sriZPgaJpZM4Pt3Cd .

-- Oliver Ruebenacker Senior Software Engineer, Diabetes Portal http://www.type2diabetesgenetics.org/, Broad Institute http://www.broadinstitute.org/

-- Oliver Ruebenacker Senior Software Engineer, Diabetes Portal http://www.type2diabetesgenetics.org/, Broad Institute http://www.broadinstitute.org/