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

feat(examples): add rolist #3400

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(examples): add rolist #3400

wants to merge 4 commits into from

Conversation

moul
Copy link
Member

@moul moul commented Dec 22, 2024

  • add avl/rolist
  • add I... interfaces for avl/...'s main structs.

Signed-off-by: moul <94029+moul@users.noreply.github.com>
@moul moul self-assigned this Dec 22, 2024
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Dec 22, 2024
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 22, 2024

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: moul/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

moul added 3 commits December 22, 2024 23:53
Signed-off-by: moul <94029+moul@users.noreply.github.com>
Signed-off-by: moul <94029+moul@users.noreply.github.com>
Signed-off-by: moul <94029+moul@users.noreply.github.com>
@moul moul marked this pull request as ready for review December 22, 2024 23:07
@moul moul requested a review from leohhhn December 22, 2024 23:07
@@ -0,0 +1,119 @@
// Package rolist provides a read-only wrapper for list.List with safe value transformation.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is list.list exactly? It's not obvious from this package.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's the gno.land/p/demo/avl/list.List struct.

Comment on lines +15 to +21
// // Create and populate the original list
// privateList := list.New()
// privateList.Append(&User{
// Name: "Alice",
// Balance: 100,
// Internal: "sensitive",
// })
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we're deviating from what we generally want with avl. If someone wants to use a tree, let them use avl and its helpers. If not, they're welcome to use the native slice. I think people won't understand when to use the native slice vs this kind of package, with an underlying avl tree.

If the point of using this kind of package because of the "safe" functionality, maybe this package should be named differently & not be under avl.

Copy link
Member Author

@moul moul Dec 28, 2024

Choose a reason for hiding this comment

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

Yes and no. For instance, I've opened an alternative version of an efficient structure to manage lists here: #3407.

However, the comments on my PR suggest there may be valid reasons to use the avl/list (the main one is about soft vs hard delete). Additionally, the alternative aims to embrace the Unix philosophy and Golang composability, demonstrating that we can build layers upon layers.

I plan to use my #3407 alternative in most cases. However, some people are still using a standard AVL map as a list; I believe you're doing this too for the hof realm, for example. Whenever you see someone using an avl.Tree with a seqid, there's a good chance they will prefer this package. For others, my ulist alternative might be a better fit.

I believe we should have such packages, but we need to:

  1. Include great comments (let me know if I should improve them here or elsewhere).
  2. Provide excellent examples—let's create more r/docs, including one comprehensive guide for all the data structures for easier comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: 📥 Inbox
Status: Triage
Development

Successfully merging this pull request may close these issues.

3 participants