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(azure-storage-blob): add raw support #442

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

blowsie
Copy link

@blowsie blowsie commented May 8, 2024

πŸ”— Linked issue

#441

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Adds support for getItemRaw in azure-storage-blob

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Functions are not documented

@pi0
Copy link
Member

pi0 commented May 8, 2024

Thanks for PR!

  • I think most of get/raw logic can now be shared using a function and we call toString for not raw version
  • We need to also implement the setItemRaw method otherwise core will stringify when setting but not deserialize in getRaw
  • Can you help to add an additional test to driver test that covers this? (/cc @itpropro he might be able to help πŸ™πŸΌ )

@pi0 pi0 marked this pull request as draft May 8, 2024 16:51
@pi0 pi0 changed the title feat: azure-storage-blob add getItemRaw support feat(azure-storage-blob): add raw supporty May 8, 2024
@pi0 pi0 changed the title feat(azure-storage-blob): add raw supporty feat(azure-storage-blob): add raw support May 8, 2024
@pi0 pi0 requested a review from itpropro May 8, 2024 16:52
Copy link

codecov bot commented May 8, 2024

Codecov Report

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

Project coverage is 66.33%. Comparing base (4d61c78) to head (a2112b4).
Report is 41 commits behind head on main.

Files Patch % Lines
src/drivers/azure-storage-blob.ts 6.66% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
+ Coverage   65.05%   66.33%   +1.27%     
==========================================
  Files          39       39              
  Lines        4055     4117      +62     
  Branches      487      509      +22     
==========================================
+ Hits         2638     2731      +93     
+ Misses       1408     1377      -31     
  Partials        9        9              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@blowsie
Copy link
Author

blowsie commented May 8, 2024

Thanks for PR!

  • I think most of get/raw logic can now be shared using a function and we call toString for not raw version

I wasn't sure about extracting and reusing the fn, since when I reviewed all the other getItemRaw implementations there is no re-use.
For this reason, I tried to copy the existing style.

  • We need to also implement the setItemRaw method otherwise core will stringify when setting but not deserialize in getRaw

I unfortunately don't know enough about how to do that, but can do some digging at some point if it helps.

  • Can you help to add an additional test to driver test that covers this? (/cc @itpropro he might be able to help πŸ™πŸΌ )

Adding tests, would be a separate issue and not a blocker for this change, right?

@pi0
Copy link
Member

pi0 commented May 8, 2024

For setItemRaw, it seems that our driver implementation will be the same. But we need to set it. see here for what happens.

Adding tests, would be a separate issue and not a blocker for this change, right?

Since this is a live driver with already tests, i prefer that we add a reasonable test that verifies this feature.

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