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

test: refactor kvpair, add tests #1057

Merged
merged 4 commits into from
Sep 6, 2023
Merged

test: refactor kvpair, add tests #1057

merged 4 commits into from
Sep 6, 2023

Conversation

wolkykim
Copy link
Contributor

  • simplify Less() and remove the unnecessary panic check
  • add comments
  • add tests

@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Aug 17, 2023
@wolkykim wolkykim changed the title Refactor kvpair, Add tests test: refactor kvpair, add tests Aug 17, 2023
@moul moul requested a review from piux2 August 21, 2023 15:45
@waymobetta
Copy link
Contributor

Hi @wolkykim, thanks for your contribution, the newly introduced comments and the tests that you've provided. The team will review this asap.

If you haven't yet, check out the Game of Realms to understand more about the contribution campaign we are running and be sure to register in order to be eligible for rewards.

@wolkykim
Copy link
Contributor Author

@waymobetta Hi, that's great to know. What is Gno.land Address in the registration?

@waymobetta
Copy link
Contributor

waymobetta commented Aug 23, 2023

What is Gno.land Address in the registration?

Example: g1de21kk9jqjkscd0hau14r8e4n52jqze231zuv3
^ Gno.land addresses include the prefix: g1

This is the public address of the wallet you've created. We collect this and associate it with your Github username in order to distribute Game of Realms rewards based on your contributions to the various Gno repositories.

@piux2
Copy link
Contributor

piux2 commented Aug 24, 2023

@wolkykim Thank you for the contribution. The testing cases and correction on comments are good!

However we are in favor of switch then if-else for Less() implementation due to following reasons.

  • Readability: switch provides a clear layout for each outcome of bytes.Compare.
  • Performance: Compilers optimize switch statements more efficiently, when the cases are dense integer values.( -1, 0, 1)
  • Explicitness: The default case in the switch acts as a safeguard against unexpected outcomes, making error handling more explicit.

@wolkykim
Copy link
Contributor Author

wolkykim commented Aug 24, 2023

@piux2 Hi, I'm also fine with using switch in general. Just that in this particular case it has a default that will never be reached, and then this line doesn't apply the same practice. Just so you know where this started.
https://github.com/gnolang/gno/pull/1057/files#diff-44b3edb2e2fef129ab0359644b4289884ffc97d958a64d2052a1c7269ac8d32bL26

Would you like this PR closed or a revised commit that rolls back the Less() to the original logic?
Let me know what you think. Thanks for the feedback.

@piux2
Copy link
Contributor

piux2 commented Aug 25, 2023

@piux2 Hi, I'm also fine with using switch in general. Just that in this particular case it has a default that will never be reached, and then this line doesn't apply the same practice. Just so you know where this started. https://github.com/gnolang/gno/pull/1057/files#diff-44b3edb2e2fef129ab0359644b4289884ffc97d958a64d2052a1c7269ac8d32bL26

Thank for pointing it out. It's good practice to include a default case even if it seems unreachable. This serves as a safety precaution. The second comparison directly returns results to the caller, which is a different scenario than the first comparison in the switch

Would you like this PR closed or a revised commit that rolls back the Less() to the original logic? Let me know what you think. Thanks for the feedback.

@wolkykim can you roll back the Less() to the original implementation. Let's keep the test and comments. Again, thank you for the contribution!

@wolkykim
Copy link
Contributor Author

@piux2 I've updated it. The original Less() logics are back to life!

@wolkykim wolkykim closed this Sep 2, 2023
@wolkykim wolkykim reopened this Sep 2, 2023
@moul moul merged commit 4586060 into gnolang:master Sep 6, 2023
63 checks passed
@wolkykim wolkykim deleted the kvpair branch September 6, 2023 12:40
@moul moul added this to the 💡Someday/Maybe milestone Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants