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: added string-store #118

Merged
merged 1 commit into from
Jul 26, 2024
Merged

feat: added string-store #118

merged 1 commit into from
Jul 26, 2024

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented May 24, 2021

TODO

  • Write remaining tests
  • Write examples in README
  • Write documentation
  • Handle edge cases
    • Invalid offset (writing outside buffer length)
    • Invalid offset (reading outside buffer length)
    • Invalid fixed-length array input (not an array, mismatching length)

@favna favna force-pushed the main branch 2 times, most recently from 1b1ed93 to a3e5cf0 Compare August 11, 2021 23:30
@Lioness100

This comment was marked as resolved.

@favna favna force-pushed the feat/added-string-store branch 2 times, most recently from db89866 to 1680a5b Compare January 20, 2022 21:26
@favna favna added this to the Future milestone Jul 24, 2022
@kyranet kyranet force-pushed the feat/added-string-store branch from 4faef40 to 10daf42 Compare July 24, 2022 21:01
favna
favna previously requested changes Jul 27, 2022
@kyranet kyranet force-pushed the feat/added-string-store branch from ed9f6c8 to 276e979 Compare July 12, 2024 22:42
@kyranet
Copy link
Member Author

kyranet commented Jul 12, 2024

This PR has been rewritten from the ground up, all reviews from the previous implementation are stale.

@kyranet kyranet force-pushed the feat/added-string-store branch from 276e979 to b246365 Compare July 12, 2024 22:45
@kyranet kyranet dismissed stale reviews from favna and vladfrangu July 12, 2024 22:45

stale

@Lioness100

This comment was marked as resolved.

Copy link

codecov bot commented Jul 12, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@kyranet kyranet force-pushed the feat/added-string-store branch from 3708a7e to 8c20b66 Compare July 13, 2024 09:15
@kyranet kyranet marked this pull request as ready for review July 13, 2024 09:15
@kyranet kyranet force-pushed the feat/added-string-store branch from 8c20b66 to c95f586 Compare July 13, 2024 09:28
@kyranet kyranet requested review from favna and vladfrangu July 13, 2024 09:34
Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

Also needs updating:

  • .github/labels-yml
  1. Is there a way to add a whole object so new Schema(Id) is valid somehow? It's a bit weird to be adding Obj.key for every single key IMHO

  2. Few people will know what all of int2/4/8/16/32 etc mean, especially those who don't know C-based languages. I count myself among them in fact. Please describe them better.

@kyranet kyranet force-pushed the feat/added-string-store branch from c95f586 to e1a9910 Compare July 13, 2024 16:26
@kyranet
Copy link
Member Author

kyranet commented Jul 13, 2024

.github/labels-yml already contained that label, and did so since db64a22

Is there a way to add a whole object so new Schema(Id) is valid somehow? It's a bit weird to be adding Obj.key for every single key IMHO

I chose the chainable method since it's a builder in builders, but Zod/Shapeshift-like objects can be done in a follow-up PR, this version is good enough as-is.

Few people will know what all of int2/4/8/16/32 etc mean, especially those who don't know C-based languages. I count myself among them in fact. Please describe them better.

int2 and int4 don't exist in any language (to my knowledge), they make sense in string-store because the unaligned buffer storage makes it take less space than an aligned one (which would store both as an int8 at minimum).

That aside, I wrote some more documentation, I hope that explains it.


By the way, after landing this PR, I'd like to hold off the v1.0.0 release, I want a first version in so I can work on QoL and details without having them around a massive PR, that way they're easier to review and to iterate.

@kyranet kyranet requested a review from favna July 13, 2024 16:33
favna
favna previously approved these changes Jul 13, 2024
favna
favna previously approved these changes Jul 15, 2024
@kyranet kyranet force-pushed the feat/added-string-store branch from c49a562 to ea90b5c Compare July 22, 2024 20:30
@kyranet kyranet force-pushed the feat/added-string-store branch from ea90b5c to 36c3695 Compare July 22, 2024 20:32
@kyranet kyranet requested a review from favna July 22, 2024 20:36
@kyranet kyranet merged commit 4c3de5e into main Jul 26, 2024
17 checks passed
@kyranet kyranet deleted the feat/added-string-store branch July 26, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants