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

Rename 'get'/'put' Methods in Clarity #4521

Merged
merged 2 commits into from Mar 13, 2024

Conversation

ASuciuX
Copy link
Contributor

@ASuciuX ASuciuX commented Mar 12, 2024

Description

Renamed get/put/put_all etc. methods in clarity and stackslib/src/clarity_vm folders, in MarfedKV, RollbackWrapper, ClarityDatabase and ClarityBackingStore structures to get_data/put_data/put_all_data etc. in order to help reduce confusion between all of the methods.

Closes #4443

@ASuciuX ASuciuX requested a review from cylewitruk March 12, 2024 11:56
@ASuciuX ASuciuX self-assigned this Mar 12, 2024
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

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

This looks good to me. I feel that this markedly helps to de-confuse the "Clarity" vs "MARF" data access concerns when navigating the codebase -- where Clarity-related accesses use the get_data/put_data/etc. and get_metadata/put_metadata/etc. paradigms which refer to Clarity DB's consensus-critical data_table and non-consensus-critical metadata_table, while the MARF itself retains get/put/etc. and deals with marf-specific tables (i.e. marf_data) and trie blob storage.

I'm curious to hear others thoughts as well.

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 74.39024% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 83.26%. Comparing base (067633d) to head (c4af902).
Report is 28 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4521      +/-   ##
==========================================
+ Coverage   83.22%   83.26%   +0.04%     
==========================================
  Files         452      452              
  Lines      326058   326069      +11     
  Branches      323      323              
==========================================
+ Hits       271359   271505     +146     
+ Misses      54691    54556     -135     
  Partials        8        8              
Files Coverage Δ
clarity/src/vm/database/key_value_wrapper.rs 86.49% <100.00%> (+0.11%) ⬆️
clarity/src/vm/database/structures.rs 80.93% <100.00%> (ø)
stackslib/src/chainstate/stacks/boot/mod.rs 94.74% <100.00%> (ø)
stackslib/src/net/api/getaccount.rs 90.96% <100.00%> (ø)
stackslib/src/net/api/getcontractsrc.rs 90.62% <100.00%> (ø)
stackslib/src/net/api/getmapentry.rs 89.28% <100.00%> (ø)
stackslib/src/clarity_vm/database/mod.rs 73.62% <66.66%> (ø)
stackslib/src/clarity_vm/database/marf.rs 65.24% <66.66%> (ø)
clarity/src/vm/database/clarity_store.rs 77.40% <37.50%> (ø)
stackslib/src/net/api/getdatavar.rs 87.85% <16.66%> (-2.59%) ⬇️
... and 1 more

... and 27 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 067633d...c4af902. Read the comment docs.

@ASuciuX ASuciuX enabled auto-merge March 13, 2024 16:12
@ASuciuX ASuciuX added this pull request to the merge queue Mar 13, 2024
Merged via the queue into stacks-network:next with commit d458b74 Mar 13, 2024
2 checks passed
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.

None yet

3 participants