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.

Sign up for GitHub

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

Jump to bottom

Prefix consolidation #439

Merged
merged 6 commits into from
Sep 30, 2021
Merged

Prefix consolidation #439

merged 6 commits into from
Sep 30, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Sep 20, 2021 •
edited
Loading

Builds on top of #434.

Uses one Prefix impl for both range and range_de. At the expense of a "dummy" deserialization to Vec<u8>.

@maurolacy maurolacy mentioned this pull request Sep 20, 2021
Merged
14 tasks
ethanfrey
ethanfrey reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

A first pass on reviewing.

It seems like you are adding proper functionality. The implementation doesn't look terribly clean.

Maybe you can just improve it after vacation. The ideas are here and just need some time to get polished.

packages/storage-plus/src/de.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/de.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/de.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/map.rs Outdated Show resolved Hide resolved
@ethanfrey ethanfrey mentioned this pull request Sep 21, 2021
Closed
ueco-jb
ueco-jb reviewed Sep 21, 2021
View reviewed changes
Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

There are plenty of seemingly magic numbers in Deserializable methods which I would rather see exaplained either in some comment(s) or in named variables.

packages/storage-plus/src/de.rs Outdated Show resolved Hide resolved
Base automatically changed from 198-prefix-de to 198-better-range-prefix September 24, 2021 06:45
@maurolacy maurolacy marked this pull request as ready for review September 30, 2021 10:57
@maurolacy
Copy link
Contributor Author

Will merge this into #432 after CI checks, if no objections are raised.

ueco-jb
ueco-jb approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

No objections, I'd rather review full changeset in #432

@maurolacy maurolacy merged commit a95f9ca into 198-better-range-prefix Sep 30, 2021
@maurolacy maurolacy deleted the 198-prefix-consolidation branch September 30, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanfrey ethanfrey ethanfrey left review comments

@ueco-jb ueco-jb ueco-jb approved these changes

@hashedone hashedone Awaiting requested review from hashedone

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maurolacy @ethanfrey @ueco-jb