ome / omero-insight

Gradle project containing insight java client for OMERO
https://www.openmicroscopy.org/omero/
GNU General Public License v2.0
7 stars 14 forks source link

equals() comparing objects of different types , 2'nd edition #417

Closed scuniff closed 8 months ago

scuniff commented 8 months ago

https://github.com/ome/omero-insight/blob/21b72187f896c36e356dfa6c4c3116bc4b633ba2/src/main/java/org/openmicroscopy/shoola/agents/metadata/editor/EditorComponent.java#L183C8-L183C58

Looks like at line 183 that 2 different objects are being used with the equals() method:

!link.canDelete() && link.getChild().equals(type))

Should the code be?

child = (AnnotationData) link.getChild(); !link.canDelete() && child.getClass().equals(type))

like it is at lines 160-162? child = (AnnotationData) link.getChild(); //Exclude some file is tag if (child.getClass().equals(type) &&

Variable type is java.lang.Class

joshmoore commented 8 months ago

@scuniff: do you foresee opening PRs for these issues?

scuniff commented 8 months ago

Yes.

I guess I’ve been kinda hesitant on doing PR’s until I’ve been told too. I’m a slow learner. :slightly_smiling_face:

In the future, should I go ahead and do a PR if I think my code will work or wait until a “code review” comment is made in the issue comments?

Thanks! Steve

scuniff commented 8 months ago

I do see Dominik has made several changes already.

Thank you

joshmoore commented 8 months ago

I guess I’ve been kinda hesitant on doing PR’s until I’ve been told too. I’m a slow learner. 🙂

I find that hard to believe :) All your found issues have been valid as far as I remember.

In the future, should I go ahead and do a PR if I think my code will work or wait until a “code review” comment is made in the issue comments?

From our side, there's certainly no reason to not open a PR right away. I would just include the (very useful) information that you've been putting in the issues in the description of the PRs itself.

Thanks so much.

jburel commented 8 months ago

Fixed in #419