-
Notifications
You must be signed in to change notification settings - Fork 161
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
[SCHEMA] Add remaining enum definitions #1640
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1640 +/- ##
==========================================
+ Coverage 87.83% 87.97% +0.14%
==========================================
Files 16 16
Lines 1356 1356
==========================================
+ Hits 1191 1193 +2
+ Misses 165 163 -2 ☔ View full report in Codecov by Sentry. |
I think we don't have to add anything for:
What do you think @effigies @Remi-Gau @tsalo ? EDIT: If you clone my branch (this PR), you can search the repo ( |
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.
Okay, I have added all definitions that I think could potentially make sense to have.
That said, this doesn't mean that all the definitions I added are meaningful beyond what the "value" says. But that's up to add for people who work with this data -- with this PR we at least:
- have added all sensible definitions and thus the infrastructure to improve the descriptions more easily (no more digging, just one search and edit the description)
- have tracked the enums for which a definition would probably not make a lot of sense with inline comments (see [SCHEMA] Add remaining enum definitions #1640 (comment))
I'd be happy for input and for help solving the CI issue 🙂
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.
Thanks @effigies! what is your general take on this PR?
Sorry, I don't have a general take. At this point I was just trying to get it working. Please bug me to think about this again next week. |
cc @effigies |
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 seems fine as-is. I don't think we need to define meter, mm and cm and mass replace those. I think the ultimate end-point of these would be to turn this into an actual ontology, with relationships between fields and values, and we need to be careful about reinventing wheels. The more we elaborate the schema, the more we're going to need to reconsider our "lightweight" custom approach versus some existing effort.
Not sure if you're looking for anything more specific in a general take.
Thanks @effigies that's the granularity I was expecting for the take I asked for 🙏 |
specifically, I intend to add definitions for those "enums" that don't have one yet.
See @tsalo and my discussion here: #919 (comment)