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

Equals&hashCode fixup #1644

Closed

Conversation

nkrzykala
Copy link
Contributor

This pull request is according to #1504

I separated modified and added files into commits by categories. You can find commits dividing:

  • classes that I used instanceof + final in and corresponding test classes
  • classes that I added canEqual to and corresponding test classes
  • classes with ignored fields (excluding "coapRequest") in the EqualsVerifier tests and corresponding test classes (because maybe the ignored fields shouldn't be avoided at some point in the future)
  • classes which had not final fields (and in EqualsVerifier we suppressed the warning about it - we may deal with it in the future as mentioned in the issue) and corresponding test classes
  • classes which had not final fields (and we made them final) and corresponding test classes
  • pom files with added dependencies
  • fixup, fe. deleting unused variables, sorting imports and adding headers

@sbernard31
Copy link
Contributor

I hope you will not be too afraid with all those comments ! Not the best way to welcome back 😁

The idea to separate PR in several commits was good but with hindsight maybe we should do that more step by step.
Maybe we several small PR with just some classes :

  • you would have received fewer comments, but more frequently (which might have avoided lot of "same kind" comments)
  • and also I would be provide better review. (It's possible that I missed some point because to much to review at same time 😅)

Reading you code, I also ask myself how Attribute class hierarchy should be compared 🤔 but this is out of scope of this PR.

@sbernard31
Copy link
Contributor

Reading Objects.hash(Arrays.hashcode()), I see that HkdfAlgorithm has no hashcode/equals method but is used in OscoreSetting which could lead to expected result 🤔 ?

@nkrzykala
Copy link
Contributor Author

Could you please explain #1644 (comment), I don't understand what you mean? That we don't know how HkdfAlgorithm will be hashed because it doesn't define equals/hashcode?

@sbernard31
Copy link
Contributor

Could you please explain #1644 (comment), I don't understand what you mean? That we don't know how HkdfAlgorithm will be hashed because it doesn't define equals/hashcode?

I added this comment just to remember that I just see that HkdfAlgorithm doesn't have custom equals/hashcode and I think we don't want to keep the default behavior.

@nkrzykala
Copy link
Contributor Author

Oh, OK sorry for misunderstanding 😄

@nkrzykala
Copy link
Contributor Author

All mentioned changes are implemented. I know that, as you mentioned in #1644 (comment), I should have created separate PRs... should I do that now or stick with this PR?

@sbernard31
Copy link
Contributor

I should have created separate PRs

My fault too, I should advice you to do that.

should I do that now or stick with this PR?

As you prefer. As I said small PR you will have better and faster feedback.
But now you have the whole code done, this will maybe be painful to split it.

@sbernard31
Copy link
Contributor

@nkrzykala is this ready for review ?

@nkrzykala
Copy link
Contributor Author

@nkrzykala is this ready for review ?

Yes, sorry I didn't let you know

@nkrzykala
Copy link
Contributor Author

Fixed those 3 issues

@nkrzykala
Copy link
Contributor Author

Hi, I forgot to do rebase of my branch onto master before I pushed last commits - now my branch is behind by 4 commits:

  • Update demo webapp dependencies (again)
  • Add very simple Queue mode support to Leshan Server Demo.
  • Fix demo-server about unexpected "Device is not awake" message
  • Fix NPE, log warn message when no registration on notification.

I am not sure how to fix that as now locally I have the right version (those 4 commits + my commits, after rebase) and remotely I have all of my commits and not those 4 ones. I have question if you could help me clean that?

@sbernard31
Copy link
Contributor

Hi, I forgot to do rebase of my branch onto master before I pushed last commits - now my branch is behind by 4 commits:

Not a big deal, if there is not so much conflict I can do that on myself.

I am not sure how to fix that as now locally I have the right version (those 4 commits + my commits, after rebase) and remotely I have all of my commits and not those 4 ones. I have question if you could help me clean that?

So generally, I create another local branch for the current remote. (so If I do something wrong and can easily go back to that state)
Then once I did all my history rewriting locally and I have my local branch in the wanted state, I just push -force the branch.

Let me know if you want to try to do that OR if I do it myself when I will integrate it in master ?

(I fetched your current remote code on my machine, so if something goes wrong I have a backup 😉)

@nkrzykala
Copy link
Contributor Author

If it wouldn't be much work and a problem - I'd prefere you to take care of it. I don't think I trust myself with that 😄 Thank you very much!

@nkrzykala
Copy link
Contributor Author

nkrzykala commented Sep 23, 2024

Hi, while looking at possible solutions for another issue (#1279) I found out that there is an old implementation of EqualsVerifiers test for Version class in VersionTest.java. I missed to delete it because my implementation of this test is in Lwm2mTest.

@sbernard31
Copy link
Contributor

I found out that there is an old implementation of EqualsVerifiers test for Version class in VersionTest.java. I missed to delete it because my implementation of this test is in Lwm2mTest.

Thx for letting me know that. I will look at that.

sbernard31 added a commit that referenced this pull request Sep 23, 2024
@sbernard31
Copy link
Contributor

sbernard31 commented Sep 23, 2024

So,

I integrated this PR in master (commit : 70eec7d)

About importing junit5 "correctly", this is fixed by 6293523)

About adding virtualHost to IpPeer equals/hashcode(), this is fixed by 47890dd

About VersionTest vs LwM2mTest, this is fixed by : ccd7bc9

So I think we can close this issue.

Thx a lot @nkrzykala for your contribution ! Really Good Job ! I really appreciate it 🙏

@sbernard31 sbernard31 closed this Sep 23, 2024
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

Successfully merging this pull request may close these issues.

2 participants