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: collections API #894

Merged
merged 13 commits into from
Sep 14, 2022
Merged

feat: collections API #894

merged 13 commits into from
Sep 14, 2022

Conversation

testinginprod
Copy link
Collaborator

@testinginprod testinginprod commented Sep 13, 2022

Description

Introduces the unified state layer

Purpose

Makes state handlings things easier and safer and standardized.
closes: #895

@testinginprod testinginprod requested a review from a team as a code owner September 13, 2022 18:41
collections/keys/numeric.go Outdated Show resolved Hide resolved
collections/keys/pair_test.go Outdated Show resolved Hide resolved
collections/keys/range.go Outdated Show resolved Hide resolved
collections/keys/range.go Outdated Show resolved Hide resolved
collections/keys/range.go Outdated Show resolved Hide resolved
collections/keys/range.go Outdated Show resolved Hide resolved
collections/keys/range.go Show resolved Hide resolved
@testinginprod testinginprod changed the title add: collections API feat: collections API Sep 14, 2022
@testinginprod
Copy link
Collaborator Author

please don't merge master as of now in this branch ty.

@NibiruHeisenberg NibiruHeisenberg self-requested a review September 14, 2022 16:02

objs := []KeyValue[keys.StringKey, wellknown.BytesValue, *wellknown.BytesValue]{kv("a"), kv("aa"), kv("b"), kv("bb")}

m.Insert(ctx, "a", obj("a"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this Insert redundant since kv("a") creates the obj already?

collections/keys/range.go Outdated Show resolved Hide resolved
// Order defines the ordering of keys.
type Order uint8

const (
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 not sure if we want to support other iteration patterns (e.g. like random access) since they are non-deterministic. I would consider if a boolean for AccessOrder is simpler than an iota. Something like true = ascending, false = descending.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the intention is to have only two iteration orders. It's just that in go there are no enums, and having booleans in constants that work like enums is not common, so I went for u8 which is the smallest value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IDK if @AgentSmithMatrix has some other feedback.

@testinginprod
Copy link
Collaborator Author

@NibiruHeisenberg , @AgentSmithMatrix there's also another point that I would like to understand before merging this. A lot of Mapping methods want to map primitive types:

  • StringKey => uint64
  • StringKey => string.

Currently the only way to do this is to use prototypes BytesValue StringValue etc.

Which make setting/getting/using the values verbose ex: m.Insert(ctx, key, gogotypes.Uint64Value{Value: 0})

For the sake of simplification we can:

  • make StringKey/Uint64Key/etc implement Object which makes them valid values for Map and Item.
  • create a secondary package (wellknown or values) which contains those types.

@testinginprod testinginprod enabled auto-merge (squash) September 14, 2022 19:41
@testinginprod testinginprod merged commit 1bd56e7 into master Sep 14, 2022
@testinginprod testinginprod deleted the mercilex/collections2 branch September 14, 2022 19:51
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.

feat(state): collections API
2 participants