-
Notifications
You must be signed in to change notification settings - Fork 10
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
Refactor validators #204
Refactor validators #204
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
=======================================
Coverage 99.68% 99.68%
=======================================
Files 11 12 +1
Lines 639 643 +4
=======================================
+ Hits 637 641 +4
Misses 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good call, but I'm slightly wary of over-nesting modules. What do you think about movement.io.file_validators.py
and movement.io.dataset_validators.py
?
Alternatively, we could keep your names but move the validators folder out of io
(thought currently they are mainly used for I/O): so movement.validators.files
and movement.validators.datasets
.
Now that I've thought more about it, I think |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Suggestion to split validators into two modules.
What is this PR
Why is this PR needed?
We are thinking of ways to make it easier for contributors to add support to new files. Since this is a part of the codebase that may be visited often by external contributors, it may be nice to make it accessible and readable.
What does this PR do?
Splits the validators into two modules:
movement.io.validators.datasets
: holds the classes for validating data intended for amovement
dataset.ValidPosesDataset
, but we are planning to add aValidBboxesDataset
soon (see Add a ValidBboxesDataset class #201).movement.io.validators.files
: holds the classes for validating files.References
\
How has this PR been tested?
Tests pass locally and on CI.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
I modified the API reference to show validators for files and datasets separately, but open to suggestions if something else is preferred.
Checklist: