[SCHEMA] Add patterns for format validation

[tsalo]

Closes #832.

This currently doesn't include any code that would use the patterns, except for tests.

Changes proposed:

• Add formats.yaml file to schema, with regular expressions with which to validate different entity values, metadata types, and metadata string formats. Individual metadata fields should still be allowed to supply a specific "pattern" keyword.

To do:

• write tests (post-[SCHEMA] Reorganize schema code into a package #892)
• add rrid pattern (for [SCHEMA] Add TSV column files #827 and the existing SoftwareRRID)
• review the URI and path patterns

[tsalo]

The patterns for all four of the path/URI formats should be revised. I don't know enough about the rules to define solid patterns just yet.

[tsalo]

The same goes for stimuli_relative (added after the initial comment).

[Remi-Gau]

This will for sure be superuseful. I don't speak regex fluently so not sure I am the best to review this though.

[rwblair]

Structure makes sense to me. Eyeballing the regex it seems ok, but the best way to verify it is for it to be used by the validator in tests.

[tsalo]

Thanks for taking a look. I'm thinking that I should try writing some unit tests for the schema code, which will necessitate reorganizing it into a proper package. I will open a PR to do that after #883 is merged, so that there aren't too many merge conflicts.

EDIT: I've opened #892.

[sappelhoff]

This is related to #876

This should allow any free-form string except "n/a".

I am not sure whether disallowing n/a as a string for free-form text fields would break some datasets. For example for the required EEGReference field, users could currently pass n/a according to bids-validator:

https://github.com/bids-standard/bids-validator/blob/75c54e4f7a17bf58c7cfc47c9698eb4e0841315b/bids-validator/validators/json/schemas/eeg.json#L25

Whether or not that's a good idea should be discussed in #876, this is just a note that what you introduce here has not been discussed and has not been formally part of the spec 🤔

[tsalo]

My hope is that any cases where "n/a" is acceptable will have "n/a" as an explicit option within the schema. I've tried to add it as an option in all of the cases that I've seen like that, but that doesn't mean we're close to having complete coverage. I can drop any fancy "n/a" handling from this PR if you'd rather wait to tackle it within #876 though.

[tsalo]

I've dropped the special "n/a" handling in dfd3057, but I do think we will want to distinguish between "n/a"s and freeform strings in the long run.

[yarikoptic]

I was wondering on why formats.yaml and not patterns.yaml if that is what it contains really?

[tsalo]

Currently, we have pattern in the schema when there's an idiosyncratic regular expression associated with a single field, e.g.,

bids-specification/src/schema/objects/metadata.yaml

Line 1606 in 638ae5c

pattern: ^(?:2[0-3]|[01][0-9]):[0-5][0-9]:[0-5][0-9]$

Whereas common "patterns" are consolidated as "formats." I guess you could say that this file defines the "patterns" associated with the "formats", but I feel like naming it patterns.yaml might be confusing since the existing "patterns" in the schema are not included in this file.

[yarikoptic]

I guess the next step for this pr is adding some tests as @tsalo was planning todo (and #833 is merged) or that would be postponed till later?

[codecov]

Codecov Report

Merging #885 (4a0ad3f) into master (51e9f1b) will increase coverage by 2.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   34.05%   36.35%   +2.30%     
==========================================
  Files           8        8              
  Lines         834      850      +16     
==========================================
+ Hits          284      309      +25     
+ Misses        550      541       -9     

Impacted Files	Coverage Δ
tools/schemacode/schemacode/tests/test_schema.py	100.00% <100.00%> (ø)
tools/schemacode/schemacode/_version.py	38.90% <0.00%> (+2.59%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e53a367...4a0ad3f. Read the comment docs.

[yarikoptic]

I just love testing boundary conditions I guess ;)

[tsalo]

It looks like ftp:// passes validation for the uri format, unfortunately. Not sure how to update the regex appropriately.

EDIT: URIs can't really be validated well with regular expressions, apparently, so I think the best solution would be to do only light testing for the regular expression, but mention in the description that folks should use an appropriate function in practice.

[yarikoptic]

It looks like ftp:// passes validation for the uri format, unfortunately. Not sure how to update the regex appropriately.

I can try later to create one following the "Syntax" of https://en.wikipedia.org/wiki/Uniform_Resource_Identifier which would demand to have a non-degenerate authority after //

edit: although not sure if it would come up stringent enough to identify errors. There is also a regex given on https://www.ietf.org/rfc/rfc3986.txt as ^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))? but that one is not stringent enough either:

In [3]: re.match("^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?", "ftp://")
Out[3]: <re.Match object; span=(0, 6), match='ftp://'>

In [4]: re.match("^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?", "ftp://").groups()
Out[4]: ('ftp:', 'ftp', '//', '', '', None, None, None, None)

and making authority non-degenerate shifts // somewhere deeper:

In [5]: re.match("^(([^:/?#]+):)?(//([^/?#]+))?([^?#]*)(\?([^#]*))?(#(.*))?", "ftp://").groups()
Out[5]: ('ftp:', 'ftp', None, None, '//', None, None, None, None)

didn't analyze further. Feel free to just add a note on possible need for improvement or other way to specify extra rules.

[tsalo]

Now that I have two approvals, I'm going to merge. I think we can keep improving these patterns (i.e., #999) before the next release.