Skip to content
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] Consolidate schema files by term type #883

Merged
merged 20 commits into from
Oct 5, 2021

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Sep 18, 2021

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.

@@ -1,5 +1,5 @@
---
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@@ -0,0 +1,27 @@
---
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new "rules" file for entities.

@tsalo
Copy link
Member Author

tsalo commented Sep 20, 2021

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.

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.
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this documentation clear enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so. Definitely helped me at least.

@tsalo tsalo mentioned this pull request Sep 22, 2021
2 tasks
@tsalo
Copy link
Member Author

tsalo commented Sep 24, 2021

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

@nellh
Copy link
Member

nellh commented Sep 24, 2021

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.

@Tokazama
Copy link
Member

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.

Copy link
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 suggestions.

src/schema/objects/datatypes.yaml Outdated Show resolved Hide resolved
src/schema/objects/datatypes.yaml Outdated Show resolved Hide resolved
src/schema/objects/datatypes.yaml Outdated Show resolved Hide resolved
src/schema/objects/datatypes.yaml Outdated Show resolved Hide resolved
src/schema/objects/datatypes.yaml Outdated Show resolved Hide resolved
tools/schemacode/utils.py Outdated Show resolved Hide resolved
tools/schemacode/utils.py Outdated Show resolved Hide resolved
tools/schemacode/schema.py Outdated Show resolved Hide resolved
tsalo and others added 2 commits September 29, 2021 10:42
The datatype descriptions.

Co-authored-by: Chris Markiewicz <[email protected]>
Make utility functions public.

Co-authored-by: Chris Markiewicz <[email protected]>
@tsalo tsalo requested a review from Remi-Gau October 3, 2021 16:16
Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

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

src/schema/README.md Outdated Show resolved Hide resolved
src/schema/README.md Outdated Show resolved Hide resolved
src/schema/README.md Outdated Show resolved Hide resolved
Comment on lines 90 to 92
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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

src/schema/README.md Outdated Show resolved Hide resolved
src/schema/README.md Outdated Show resolved Hide resolved
MUST have a corresponding `Density` metadata field to provide
interpretation.

This entity is only applicable to derivative data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so. Definitely helped me at least.

src/schema/README.md Outdated Show resolved Hide resolved
@tsalo
Copy link
Member Author

tsalo commented Oct 5, 2021

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
Copy link
Collaborator

Remi-Gau commented Oct 5, 2021

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
Copy link
Member

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
Copy link
Member Author

tsalo commented Oct 5, 2021

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!

@tsalo tsalo merged commit 251f440 into bids-standard:master Oct 5, 2021
@tsalo 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
I definitely made a mistake in bids-standard#883.
tsalo added a commit that referenced this pull request Oct 18, 2021
* 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.
@tsalo tsalo added 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. schema-structure Changes to the fundamental organization/structure of the YAML schema. Minor version release. labels Apr 11, 2022
yarikoptic added a commit to yarikoptic/bids-specification that referenced this pull request Dec 4, 2022
…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 ^^^
effigies pushed a commit that referenced this pull request Dec 4, 2022
…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 ^^^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. schema-structure Changes to the fundamental organization/structure of the YAML schema. Minor version release.
Projects
None yet
6 participants