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

Validation results refactoring: centralize results for different reused origins, use _pydantic_errors_to_validation_results more #1176

Closed
wants to merge 3 commits into from

Conversation

yarikoptic
Copy link
Member

See individual commits messages for more information.

Please check (especially @TheChymera ) if my use of _pydantic_errors_to_validation_results is correct -- I am not sure any test is hitting that code path at all - can someone come up with a unittest to hit it?

@yarikoptic yarikoptic added the patch Increment the patch version when merged label Dec 16, 2022
@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.15%. Comparing base (8e67482) to head (d8e0558).
Report is 669 commits behind head on master.

Files with missing lines Patch % Lines
dandi/files/bases.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
- Coverage   89.15%   89.15%   -0.01%     
==========================================
  Files          76       76              
  Lines        9510     9518       +8     
==========================================
+ Hits         8479     8486       +7     
- Misses       1031     1032       +1     
Flag Coverage Δ
unittests 89.15% <85.71%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@jwodder jwodder left a comment

Choose a reason for hiding this comment

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

Shouldn't this PR be labelled "internal" instead of "patch"? None of these changes appear to be externally visible.

dandi/files/bases.py Outdated Show resolved Hide resolved
dandi/files/bases.py Outdated Show resolved Hide resolved
Copy link
Contributor

@TheChymera TheChymera left a comment

Choose a reason for hiding this comment

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

Looks good to me (in addition to #1176 (comment) ). The partial() definitions are pretty convenient. As for tests not hitting those two lines, I guess the “problem” is that nothing in our test suite is triggering those exceptions. This looks more like failsafe code, in case something that should never break by some coincidence does 🤔 not exactly sure how to make it break without breaking in other places before.

@yarikoptic yarikoptic added internal Changes only affect the internal API and removed patch Increment the patch version when merged labels Dec 19, 2022
@yarikoptic
Copy link
Member Author

Shouldn't this PR be labelled "internal" instead of "patch"? None of these changes appear to be externally visible.

my thinking was "because it changes visible to user results" due to " use _pydantic_errors_to_validation_results more". but indeed, I guess, could just be internal.

@yarikoptic
Copy link
Member Author

This looks more like failsafe code, in case something that should never break by some coincidence does not exactly sure how to make it break without breaking in other places before.

hm, shouldn't that get_validation_errors case be triggered on any asset with invalid according to pydantic model? But let's continue on that in a separate issue

@yarikoptic
Copy link
Member Author

ok, suggestions incorporated, issue filed, this is green -- let's proceed

@yarikoptic yarikoptic closed this Dec 19, 2022
@TheChymera TheChymera deleted the rf-results branch July 3, 2023 11:35
@yarikoptic yarikoptic restored the rf-results branch July 7, 2023 22:20
@yarikoptic yarikoptic reopened this Jul 7, 2023
@yarikoptic
Copy link
Member Author

I think it might have been closed in error. Should be redone.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants