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(storage): wrapper class for versioned objects #1710

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

yair-starkware
Copy link
Contributor

@yair-starkware yair-starkware commented Feb 14, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information


This change is Reviewable

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (9c3652e) 73.35% compared to head (7c16727) 73.33%.

Files Patch % Lines
crates/papyrus_storage/src/db/serialization.rs 58.62% 7 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1710      +/-   ##
==========================================
- Coverage   73.35%   73.33%   -0.02%     
==========================================
  Files         120      120              
  Lines       16560    16589      +29     
  Branches    16560    16589      +29     
==========================================
+ Hits        12147    12166      +19     
- Misses       2608     2614       +6     
- Partials     1805     1809       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yair-starkware yair-starkware force-pushed the yair/version_wrapper branch 5 times, most recently from d5a7531 to c9b0854 Compare February 20, 2024 11:14
@dan-starkware
Copy link
Collaborator

Can you please draft the usage for version 0 and header?

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 149 of 149 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yair-starkware)


crates/papyrus_storage/src/db/table_types/simple_table.rs line 80 at r2 (raw file):

            return Ok(None);
        };
        Ok(<Self::Value>::deserialize(&mut bytes.as_ref()))

Please explain this change.

Code quote:

 Ok(<Self::Value>::deserialize(&mut bytes.as_ref()))

crates/papyrus_storage/src/mmap_file/mod.rs line 277 at r2 (raw file):

        &self,
        res: &mut impl std::io::Write,
    ) -> Result<(), crate::db::serialization::StorageSerdeError> {

Not related to this PR, consider reverting and adding a GFI.

Code quote:

crate::db::serialization::

Copy link
Contributor Author

@yair-starkware yair-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 146 of 149 files reviewed, 1 unresolved discussion (waiting on @dan-starkware)

a discussion (no related file):

Previously, dan-starkware wrote…

Can you please draft the usage for version 0 and header?

#1718



crates/papyrus_storage/src/db/table_types/simple_table.rs line 80 at r2 (raw file):

Previously, dan-starkware wrote…

Please explain this change.

reverted

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

@yair-starkware yair-starkware added this pull request to the merge queue Feb 21, 2024
Merged via the queue into main with commit 1792435 Feb 21, 2024
16 checks passed
@yair-starkware yair-starkware deleted the yair/version_wrapper branch February 21, 2024 09:07
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants