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

Match the public and private APIs for content reading #386

Merged
merged 8 commits into from
Dec 20, 2023
Merged

Conversation

icidasset
Copy link
Member

Summary

I've made it so that the public and private APIs for reading content better match.

wnfs changes:

  • Made the length param optional and renamed the offset param to match the public side.
  • Copied the doc test from the public side to the private side.
  • Made it so that PrivateFile.get_content uses its read_at method.
  • Added PublicFile.get_content to reflect the private side.

wnfs-wasm changes:

  • Added PrivateFile.read_at
  • Made it so that PrivateFile.get_content uses its read_at method.
  • Added PublicFile.get_content
  • Adjusted tests to use new and changed methods.

Test plan (required)

Added and adjusted some tests.

After Merge

  • Does this change invalidate any docs or tutorials? If so ensure the changes needed are either made or recorded
  • Does this change require a release to be made? Is so please create and deploy the release

@icidasset icidasset requested a review from a team as a code owner December 20, 2023 12:15
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

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

Comparison is base (0964ac4) 54.80% compared to head (da90280) 54.57%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   54.80%   54.57%   -0.23%     
==========================================
  Files          57       57              
  Lines        3936     3943       +7     
  Branches      947      948       +1     
==========================================
- Hits         2157     2152       -5     
- Misses       1169     1180      +11     
- Partials      610      611       +1     
Files Coverage Δ
wnfs/src/public/file.rs 69.82% <ø> (ø)
wnfs/src/private/file.rs 70.20% <70.96%> (-3.49%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

I'd take this as-is, but while "we have the hood up" and working towards public & private APIs being aligned, would you be up for tackling a small refactor from usize to u64 for byte offsets? (See the comment)

wnfs/src/private/file.rs Outdated Show resolved Hide resolved
wnfs-wasm/tests/private.spec.ts Show resolved Hide resolved
wnfs/src/private/file.rs Outdated Show resolved Hide resolved
wnfs/src/private/file.rs Outdated Show resolved Hide resolved
wnfs/src/private/file.rs Outdated Show resolved Hide resolved
@matheus23 matheus23 merged commit 502c4c3 into main Dec 20, 2023
10 checks passed
@matheus23 matheus23 deleted the read_at branch December 20, 2023 16:36
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.

2 participants