Skip to content

[SCHEMA] Add suffix term files#772

Merged
sappelhoff merged 28 commits intobids-standard:masterfrom
tsalo:suffix-schema
Jul 20, 2021
Merged

[SCHEMA] Add suffix term files#772
sappelhoff merged 28 commits intobids-standard:masterfrom
tsalo:suffix-schema

Conversation

@tsalo
Copy link
Copy Markdown
Member

@tsalo tsalo commented Apr 9, 2021 •
edited by sappelhoff
Loading

Closes None.

Changes proposed:

  • Add a new folder within the schema of YAML files describing valid BIDS file suffixes.
  • Render suffix tables with a macro instead of hardcoded tables.

To do:

  • Add functions to schemacode to render suffix description tables.
  • Remove hardcoded tables (after review).
  • Determine final structure for suffix schema.
    • Currently, I just have description, name, unit, minValue, and maxValue.
    • Should we include "valid extensions", or is that dependent on data type?
    • Should we include a deprecated flag, or is that dependent on data type?
    • One key difference from metadata is that the name field in suffixes is used to store the full name for the data stored in files with that suffix, while name in metadata files is used to differentiate between the actual field and the name of the file, so we can have multiple definitions for the same field by storing them in separate files.

@tsalo tsalo marked this pull request as draft April 9, 2021 19:22
@tsalo tsalo added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Apr 19, 2021
@tsalo tsalo marked this pull request as ready for review April 19, 2021 18:54
@tsalo tsalo requested a review from chrisgorgo as a code owner April 19, 2021 18:54
tsalo
tsalo commented Apr 19, 2021
View reviewed changes
@effigies
Copy link
Copy Markdown
Collaborator

effigies commented Jul 13, 2021

@tsalo Is this still WIP or are you interested in a review?

@tsalo
Copy link
Copy Markdown
Member Author

tsalo commented Jul 13, 2021

I'd like to tinker with it a bit before a full review once #774 is merged, since there will definitely be a few merge conflicts to deal with, but after that I'd love any and all feedback.

@tsalo
Copy link
Copy Markdown
Member Author

tsalo commented Jul 13, 2021

@effigies Okay, I think it's ready for review.

effigies
effigies reviewed Jul 14, 2021
View reviewed changes
tsalo and others added 2 commits July 14, 2021 12:18
@tsalo @effigies
Apply suggestions from code review
f453013 
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@tsalo
Further address review.
ffedc6b
@tsalo tsalo mentioned this pull request Jul 14, 2021
Closed
effigies
effigies approved these changes Jul 14, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Small changes.

@tsalo @effigies
Apply suggestions from code review
89828bc 
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
sappelhoff
sappelhoff reviewed Jul 15, 2021
View reviewed changes
sappelhoff
sappelhoff reviewed Jul 15, 2021
View reviewed changes
sappelhoff
sappelhoff approved these changes Jul 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

apart from my two comments LGTM

going from "map" to "image" was probably a conscious decision, as was going from abbreviations to full names.

@tsalo
Copy link
Copy Markdown
Member Author

tsalo commented Jul 15, 2021 •
edited
Loading

Thanks for reviewing @effigies and @sappelhoff! Now that we have two approvals, I can trigger the five-day waiting period and merge next Thursday (July 22nd) if no one raises any concerns.

sappelhoff
sappelhoff requested changes Jul 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

just a note that we still need to remove the hardcoded tables before merging.

(I think you can merge starting from today, as it's been 5 days)

@tsalo
Copy link
Copy Markdown
Member Author

tsalo commented Jul 20, 2021

Okay, the hardcoded tables are gone.

@sappelhoff sappelhoff merged commit ed1357c into bids-standard:master Jul 20, 2021
@sappelhoff
Copy link
Copy Markdown
Member

sappelhoff commented Jul 20, 2021

Thanks @tsalo

@tsalo tsalo deleted the suffix-schema branch July 20, 2021 15:00
@tsalo tsalo mentioned this pull request Jul 20, 2021
Merged
@tsalo tsalo added the schema-code Updates or changes to the code used to parse, filter, and render the schema. label Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@effigies effigies effigies approved these changes

@sappelhoff sappelhoff sappelhoff approved these changes

@chrisgorgo chrisgorgo Awaiting requested review from chrisgorgo

Assignees

No one assigned

Labels

schema Issues related to the YAML schema representation of the specification. Patch version release. schema-code Updates or changes to the code used to parse, filter, and render the schema.

Projects

Conversion to schema
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tsalo @effigies @sappelhoff