[SCHEMA] Consolidate schema files by term type by tsalo · Pull Request #883 · bids-standard/bids-specification

tsalo · 2021-09-18T21:36:33Z

Closes #877 and closes #603.

Changes proposed:

Combine the schema files in each term type (for example, datatypes, metadata fields, and suffixes) into single files.
Separate term definitions from term rules.
Update the schema code to work with the new structure.
Document the new structure in the schema folder's README.

Still need to document the rules files.

tsalo · 2021-09-18T21:39:11Z

src/schema/rules/datatypes/beh.yaml

@@ -1,5 +1,5 @@
 ---


These datatypes files are essentially unchanged. I didn't come up with a clean way to reorganize them.

I did end up changing the indents for lists because apparently yamllint's default behavior doesn't match libyaml's defaults. I don't think there's even an easy way to write out lists with that extra indent with pyyaml, so this should make it easier to write out schema files programmatically in the future.

tsalo · 2021-09-18T21:39:29Z

src/schema/rules/entities.yaml

@@ -0,0 +1,27 @@
+---


This is a new "rules" file for entities.

tsalo · 2021-09-20T15:35:58Z

Given the ~400 files that are removed or just moved, I've added comments to the most relevant ones.

EDIT: It turns out there's an option to hide deleted files under "File filters" that I was previously unaware of, so that should help.

tsalo · 2021-09-20T15:36:44Z

src/schema/README.md

-        as `type`: `minItems`, `maxItems`.
-        Here is an example of a field that MUST have three `integer` items:
-        ```yaml
+The schema is divided into two parts: the object definitions and the rules.


Is this documentation clear enough?

I think so. Definitely helped me at least.

tsalo · 2021-09-24T17:00:53Z

Tagging @bids-standard/schema-users. Is this going to cause any major problems for any tools that use the schema?

nellh · 2021-09-24T18:01:05Z

Tagging @bids-standard/schema-users. Is this going to cause any major problems for any tools that use the schema?

Looks like just path updates needed for bids-validator but otherwise it looks compatible with the use case so far.

See https://github.com/bids-standard/bids-specification/pull/885/files#r715815807.

Tokazama · 2021-09-24T23:22:31Z

I haven't finalized any schema related code yet at https://github.com/JuliaNeuroscience/NeuroStudies.jl, but this certainly makes it easier to manage and update with changes here.

effigies

LGTM. Small suggestions.

src/schema/objects/datatypes.yaml

tools/schemacode/utils.py

tools/schemacode/schema.py

The datatype descriptions. Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Make utility functions public. Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Remi-Gau

It all makes sense to me. This should definitely help a lot. Thanks @tsalo

src/schema/README.md

Remi-Gau · 2021-10-05T07:41:24Z

src/schema/README.md

+The `type` field defines the representation type for the value associated with the entity.
+All entities are represented in filenames as key-value pairs, and in all cases the value should be a string,
+so the `type` field should always be `string`.


Maybe give an example here because on its own, I find this confusing.

I rewrote this portion in 70e2c4e. I hope it's clearer now.

src/schema/README.md

Remi-Gau · 2021-10-05T08:00:28Z

src/schema/objects/entities.yaml

+    MUST have a corresponding `Density` metadata field to provide
+    interpretation.
+
+    This entity is only applicable to derivative data.


Side note: I suspect you have thought about this, but we might need at some point to create a rule to keep track of derivatives only entities.

You're right. We can always add whatever fields we want, but it's probably something that can be formalized with the changes proposed by @erdalkaraca in #884.

Remi-Gau · 2021-10-05T08:02:48Z

src/schema/README.md

-        as `type`: `minItems`, `maxItems`.
-        Here is an example of a field that MUST have three `integer` items:
-        ```yaml
+The schema is divided into two parts: the object definitions and the rules.


I think so. Definitely helped me at least.

@Remi-Gau

Thanks @Remi-Gau! Co-authored-by: Remi Gau <remi_gau@hotmail.com>

src/schema/README.md

tsalo · 2021-10-05T16:01:13Z

Now that we have two approvals and I think I've addressed the review comments, I believe this is ready to merge. Do we want to wait five days before merging?

Remi-Gau · 2021-10-05T16:07:19Z

Now that we have two approvals and I think I've addressed the review comments, I believe this is ready to merge. Do we want to wait five days before merging?

Technically this does not affect the content itself so I am not sure if the 5 days rule apply.

https://github.com/bids-standard/bids-specification/blob/master/DECISION-MAKING.md#rules

I know that there have been precedents for infra related PR where we merged sooner than that. @sappelhoff @effigies

sappelhoff · 2021-10-05T16:11:04Z

I haven't had a chance to take a look yet, but yeah - if no content is impacted and most major schema-stakeholders agree, then I think merging is fine

tsalo · 2021-10-05T17:59:56Z

Well, I pinged the schema users team 11 days ago and no one raised any concerns, so I guess I'll merge once CI finishes. Thanks!

I definitely made a mistake in bids-standard#883.

* Add new MEG structure. * Document new rules format. * Support the new structure in the code. * Fix entity tables bug. I definitely made a mistake in #883. * Clean up code. * Fix the entity table more. * Drop unnecessary MEG group. * Try something. * Handle arbitrary numbers of rows and just use duplicate name. * Forgot to revert the import change.

…les/raw Was suggestsed in one of the files within https://github.com/bids-standard/bids-specification/pull/1128/files#diff-374b38cbccac62b8efa8e4ab2552cf9be8ba9eff7c45025abfc6da27d461a37b and is needed to the recent refactoring done in 251f440 (bids-standard#883) === Do not change lines below === { "chain": [], "cmd": "git-sedi 'rules\\/datatypes' 'rules\\/files\\/raw'", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

…les/raw (#1362) Was suggestsed in one of the files within https://github.com/bids-standard/bids-specification/pull/1128/files#diff-374b38cbccac62b8efa8e4ab2552cf9be8ba9eff7c45025abfc6da27d461a37b and is needed to the recent refactoring done in 251f440 (#883) === Do not change lines below === { "chain": [], "cmd": "git-sedi 'rules\\/datatypes' 'rules\\/files\\/raw'", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

tsalo added 10 commits September 17, 2021 11:45

Reorganize schema.

922f451

Fix formatting issues.

12f180e

Start documenting new structure in README.

dedb79a

Still need to document the rules files.

Fix every indent ever.

4d91173

Fix MD spacing.

8ea6028

Update README.md

892300d

Update README.md

99bbae3

Document the rules files.

31f4dfb

Clean up the datatypes rules files.

5729b07

Get schemacode working with new structure.

eb105d3

tsalo mentioned this pull request Sep 18, 2021

Consolidate schema files into a single file for each term type #877

Closed

tsalo commented Sep 18, 2021

View reviewed changes

tsalo requested review from effigies and yarikoptic September 20, 2021 15:36

tsalo commented Sep 20, 2021

View reviewed changes

It turns out the extra indents are not strictly YAML compliant.

3e832f3

tsalo mentioned this pull request Sep 22, 2021

[SCHEMA] Add TSV column files #827

Merged

2 tasks

tsalo mentioned this pull request Sep 24, 2021

[SCHEMA] Add patterns for format validation #885

Merged

3 tasks

"date" should be a format, not a pattern.

e9c4f5f

See https://github.com/bids-standard/bids-specification/pull/885/files#r715815807.

effigies approved these changes Sep 29, 2021

View reviewed changes

tsalo and others added 2 commits September 29, 2021 10:42

Apply suggestions from code review

1760841

The datatype descriptions. Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Apply suggestions from code review

3e97c5a

Make utility functions public. Co-authored-by: Chris Markiewicz <effigies@gmail.com>

tsalo requested a review from Remi-Gau October 3, 2021 16:16

Remi-Gau approved these changes Oct 5, 2021

View reviewed changes

Apply suggestions from code review

e0949c5

Thanks @Remi-Gau! Co-authored-by: Remi Gau <remi_gau@hotmail.com>

tsalo commented Oct 5, 2021

View reviewed changes

src/schema/README.md Outdated Show resolved Hide resolved

Explain entity "types" a bit better.

70e2c4e

tsalo added 4 commits October 5, 2021 13:41

Try fixing the markdown style issues.

2627d39

Update README.md

a46ce4b

Update README.md

4ca0c57

Update README.md

ba6bc16

tsalo merged commit 251f440 into bids-standard:master Oct 5, 2021

tsalo deleted the consolidate-schema-files branch October 5, 2021 18:02

tsalo added a commit to tsalo/bids-specification that referenced this pull request Oct 14, 2021

Fix entity tables bug.

f19ef56

I definitely made a mistake in bids-standard#883.

This was referenced Oct 14, 2021

[SCHEMA] Add new MEG files and fix entity tables #898

Merged

Use a coherent schema definition syntax #884

Open

yarikoptic mentioned this pull request Dec 4, 2022

[DATALAD RUNCMD] Replace mentionings of rules/datatypes with rules/files/raw #1362

Merged

Conversation

tsalo commented Sep 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsalo commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsalo commented Sep 24, 2021

Uh oh!

nellh commented Sep 24, 2021

Uh oh!

Tokazama commented Sep 24, 2021

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remi-Gau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsalo commented Oct 5, 2021

Uh oh!

Remi-Gau commented Oct 5, 2021

Uh oh!

sappelhoff commented Oct 5, 2021

Uh oh!

tsalo commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tsalo commented Sep 18, 2021 •

edited

Loading

tsalo commented Sep 20, 2021 •

edited

Loading