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

add read_buffer_context and write_read_buffer_context #875

Merged
merged 10 commits into from
May 16, 2023

Conversation

braingram
Copy link
Contributor

@braingram braingram commented May 5, 2023

closes #873

Changes

Add read_buffer_context and write_read_buffer_context to weldx.asdf.util.

Update previous uses of read_buffer and write_read_buffer that inspect the result to use the context.

read_buffer and write_read_buffer were updated to use the new context to avoid code duplication. These were not removed to preserve the API and to leave them available for uses where the result is not inspected. Such as this test:

def test_uuid():
"""Test uuid serialization and version 4 pattern."""
write_read_buffer({"id": uuid.uuid4()})
with pytest.raises(ValidationError):
write_read_buffer({"id": uuid.uuid1()})

Related Issues

Closes #873

Checks

  • updated CHANGELOG.md
  • updated tests/
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

@CagtayFabry CagtayFabry added ASDF everything ASDF related (python + schemas) tests pytest (and other tests) related labels May 6, 2023
@github-actions
Copy link

github-actions bot commented May 6, 2023

Test Results

2 188 tests   - 1   2 187 ✔️  - 1   2m 41s ⏱️ +7s
       1 suites ±0          1 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 04ef802. ± Comparison against base commit 7b5c0c1.

This pull request removes 1 test.
weldx.tests.asdf_tests.test_asdf_util ‑ test_write_buffer_dummy_inline_arrays

♻️ This comment has been updated with latest results.

@CagtayFabry CagtayFabry self-requested a review May 10, 2023 10:21
@CagtayFabry CagtayFabry self-assigned this May 10, 2023
Copy link
Member

@CagtayFabry CagtayFabry left a comment

Choose a reason for hiding this comment

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

thank you very much for you contribution here @braingram

weldx/tests/asdf_tests/test_asdf_util.py Outdated Show resolved Hide resolved
@CagtayFabry CagtayFabry requested a review from marscher May 11, 2023 12:18
Copy link
Contributor

@marscher marscher left a comment

Choose a reason for hiding this comment

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

Thanks! The utility functions are not meant to be used directly by users, as dealing with file handle life times can be tedious. This is mainly why we introduced a wrapper class around the file handle (also using the context manager protocol).

@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Merging #875 (04ef802) into master (7b5c0c1) will decrease coverage by 0.04%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #875      +/-   ##
==========================================
- Coverage   96.51%   96.48%   -0.04%     
==========================================
  Files          94       94              
  Lines        6283     6284       +1     
==========================================
- Hits         6064     6063       -1     
- Misses        219      221       +2     
Impacted Files Coverage Δ
weldx/asdf/util.py 90.28% <87.50%> (-0.60%) ⬇️

@CagtayFabry CagtayFabry merged commit e4e563c into BAMWelDX:master May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASDF everything ASDF related (python + schemas) tests pytest (and other tests) related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

weldx.asdf.util.read_buffer relies on reading from a 'closed' buffer
3 participants