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

extra dump before decode #287

Merged
merged 8 commits into from
Oct 17, 2024
Merged

extra dump before decode #287

merged 8 commits into from
Oct 17, 2024

Conversation

volodymyrss
Copy link
Member

@volodymyrss volodymyrss commented Oct 16, 2024

includes #286

@volodymyrss volodymyrss marked this pull request as draft October 16, 2024 17:21
@volodymyrss volodymyrss changed the base branch from test_rmf to master October 16, 2024 17:24
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.32%. Comparing base (4290767) to head (77c8dfc).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
tests/test_data_products.py 87.09% 4 Missing ⚠️
oda_api/data_products.py 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage   59.01%   59.32%   +0.30%     
==========================================
  Files          23       23              
  Lines        4956     4993      +37     
==========================================
+ Hits         2925     2962      +37     
  Misses       2031     2031              

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

@volodymyrss volodymyrss marked this pull request as ready for review October 17, 2024 09:29
oda_api/data_products.py Outdated Show resolved Hide resolved
oda_api/data_products.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@burnout87 burnout87 left a comment

Choose a reason for hiding this comment

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

I guess now the tests can be removed/adapted?

if it's ok for you I can do it

@volodymyrss
Copy link
Member Author

It's important to keep the test, at least the one I added, to capture the problem and make sure it does not repeat with future updates.

The test_rmf can be updated as you prefer.
Although in principle it is a higher-level test so it's good to keep it too.
It was important to show that the patch I provided is fixing the test @ferrigno added to demonstrate.
That is, it could be updated a bit if needed, much of it was not actually triggering for me.
What do you propose exactly?

@burnout87
Copy link
Collaborator

What do you propose exactly?

Nothing in particular. Yes, to keep especially your test is important. As the test_rmf, also for me, I dont have the second exception caught too (I managed to have when running the same steps on a notebook...probably some differences in the environments used), so perhpas just that test can be adapted.

@volodymyrss
Copy link
Member Author

What do you propose exactly?

Nothing in particular. Yes, to keep especially your test is important. As the test_rmf, also for me, I dont have the second exception caught too (I managed to have when running the same steps on a notebook...probably some differences in the environments used), so perhpas just that test can be adapted.

I suppose there is a reason @ferrigno added this code, so if this motivation is written in a comment like I did, we could preserve it too.
If not useful let's reduce or drop it. Up to you and @ferrigno .

@volodymyrss volodymyrss merged commit abe37ba into master Oct 17, 2024
16 of 17 checks passed
@burnout87 burnout87 mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not find heap data for the \'MATRIX\' variable-length array column.
2 participants