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

ORM: Add from_bytes classmethod to orm.SinglefileData #6653

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

GeigerJ2
Copy link
Contributor

@GeigerJ2 GeigerJ2 commented Dec 6, 2024

As noted together with @elinscott and @mikibonacci: orm.SinglefileData supports getting its content as bytes via get_content, but did not support setting its contents from bytes directly. This is now made possible with the from_bytes classmethod. A few notes on the implementation:

  • One could also modify the existing from_string method, and adapt the behavior depending on the type of the content being passed. However, I feel like the name from_string, in that case, is misleading. It would be more suitable if the method were called from_content (or similar, more generic), however, that is not the case, and we cannot modify it due to backwards-compatibility reasons.
  • Users could, in principle, just use .decode('utf-8') themselves on the bytes object they want to store before calling .from_string(), but it might be more convenient to allow directly passing the bytes object via from_bytes, as to not have the burden of conversion on the user.
  • The functionality tested in the test_get_content function is now already being tested in the (already existing) test_from_string and the (new) test_from_bytes functions, which check the content of the SinglefileData node after storing. However, I still left it for completeness.

@GeigerJ2 GeigerJ2 changed the title Add from_bytes classmethod to orm.SinglefileData Add from_bytes classmethod to orm.SinglefileData Dec 6, 2024
@GeigerJ2 GeigerJ2 requested a review from mikibonacci December 6, 2024 09:42
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.91%. Comparing base (baf8d7c) to head (435e274).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6653      +/-   ##
==========================================
- Coverage   77.92%   77.91%   -0.00%     
==========================================
  Files         563      563              
  Lines       41668    41671       +3     
==========================================
+ Hits        32464    32465       +1     
- Misses       9204     9206       +2     

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

Copy link
Contributor

@mikibonacci mikibonacci left a comment

Choose a reason for hiding this comment

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

Hi @GeigerJ2 , LGTM! thanks a lot for the PR, it is very useful

@GeigerJ2 GeigerJ2 force-pushed the feature/singlefile-from-bytes branch from 495c52c to 435e274 Compare December 11, 2024 13:48
@GeigerJ2 GeigerJ2 changed the title Add from_bytes classmethod to orm.SinglefileData ORM: Add from_bytes classmethod to orm.SinglefileData Dec 11, 2024
@GeigerJ2 GeigerJ2 merged commit 0f0b88a into aiidateam:main Dec 11, 2024
9 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.

2 participants