-
Notifications
You must be signed in to change notification settings - Fork 1
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
Issue #271 - Allow missing values when an sdmxa:obsStatus
Attribute explains why it's missing.
#272
Conversation
… explains why it's missing.
csvcubed/csvcubed/utils/qb/cube.py
Outdated
return errors | ||
|
||
|
||
def _validate_missing_observation_values( |
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.
I think that the observations should be in their own module.
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.
I think at some point we'll end up splitting out all of the different validations into separate files. What makes you suggest it now?
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.
It's grown this module by 50% LoC, which is some artibtrary mental threshold for some minor refactoring. Won't hurt if you don't want to do it now, but it is easily done now.
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.
I think pulling the observation stuff out of the cube.py is worthwhile now; however not a dealbreaker.
# Unfortunately, CSV-W validation will *not* catch this error since the obs column cannot be marked as `required` | ||
# since an `sdmxa:obsStatus` Attribute column has been defined. | ||
Then csvlint validation of "bad-qube.csv-metadata.json" should succeed |
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.
Is there a thing to do with csvcheck that could solve this?
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.
I don't think csvw-check is the right place to solve the issue. We'd effectively be making an unofficial extension to their CSV-W spec.
However, there is definitely scope for more CSV-qb validation which I believe is in our issues long-term.
csvcubed/csvcubed/utils/qb/cube.py
Outdated
return errors | ||
|
||
|
||
def _validate_missing_observation_values( |
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.
It's grown this module by 50% LoC, which is some artibtrary mental threshold for some minor refactoring. Won't hurt if you don't want to do it now, but it is easily done now.
Issues raised as a result of this PR:
Satisfies Issue #271.