bids-specification icon indicating copy to clipboard operation
bids-specification copied to clipboard

[SCHEMA] Add full object definitions for valid values in schema

Open tsalo opened this issue 3 years ago • 4 comments

Closes #904. This PR adds new information to the schema, but should not affect how it is render in the specification.

Changes proposed:

  • Convert certain valid values (enums) in the schema to full objects. Specifically, these object definitions contain "name" and "description" fields. In most cases, they're still nested within the parent object definitions, rather than stored as distinct objects and referenced in the objects that use them.

To do:

  • [ ] Ensure that the schema rendering and searching functions can handle the fact that many lists of strings are now lists of objects.
  • [ ] Fill in the TODOs. I may need to ask for help in describing some (many?) of the values.
  • [x] ~~Think about tackling #912 in this PR as well.~~ Dropped in favor of #921.

tsalo avatar Nov 01 '21 17:11 tsalo

Codecov Report

Patch coverage: 79.54% and project coverage change: -0.18 :warning:

Comparison is base (f2cea0f) 88.00% compared to head (41189dc) 87.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
- Coverage   88.00%   87.83%   -0.18%     
==========================================
  Files          16       16              
  Lines        1326     1356      +30     
==========================================
+ Hits         1167     1191      +24     
- Misses        159      165       +6     
Impacted Files Coverage Δ
tools/schemacode/bidsschematools/render/utils.py 85.18% <0.00%> (ø)
tools/schemacode/bidsschematools/schema.py 79.61% <81.81%> (-0.18%) :arrow_down:
tools/schemacode/bidsschematools/rules.py 98.96% <83.33%> (-1.04%) :arrow_down:
tools/schemacode/bidsschematools/render/text.py 95.53% <87.50%> (-1.10%) :arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Jan 31 '22 18:01 codecov[bot]

Noting that this would be a backwards incompatible change (cf #1013), but easily handled by detecting a list of single-keyed objects to a list of keys.

effigies avatar Apr 11 '22 16:04 effigies

@tsalo Sorry, this is a nasty one to try to resolve on my own. Conflicts from both #1230 and #1235.

effigies avatar Aug 19 '22 17:08 effigies

@effigies I think it's all good now.

tsalo avatar Aug 19 '22 18:08 tsalo

Per discussion today, I will (1) resolve conflicts, (2) move the value definitions to a new YAML file, and (3) ensure that there are links in the "allowed values" sections in glossary entries.

tsalo avatar Nov 10 '22 20:11 tsalo

@sappelhoff @effigies I've added all the channel types I could find. It looks like there are already enums for valid channels types within the different modalities, so we might actually be more strict than we want to be with enforcement.

I haven't split up the values file at all. Not sure how you'd like to do that.

tsalo avatar Mar 24 '23 14:03 tsalo

Thanks for reviewing @sappelhoff!

While other items don't have such a comment, yet they are not converted to the "new form" either

Those could be enums that were added after I started this PR, or they might even be ones I just missed in my original pass-through.

Shouldn't we just add information for all enums (except those that only have "n/a")?

I think some are self-explanatory (e.g., I don't know if I want to write descriptions for all of the allowed values for the handedness column, since they include l, L, left, Left, LEFT, etc.), but I do think most valid values could use a description. I think we can tackle that in future PRs though, since this one adds in the infrastructure we need to make that work.

tsalo avatar Jun 22 '23 16:06 tsalo

Thanks!

tsalo avatar Jun 23 '23 13:06 tsalo

thanks a lot again @tsalo!

sappelhoff avatar Jun 27 '23 08:06 sappelhoff