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.

Sign up for GitHub

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

Jump to bottom

Add DatasetLinks to JSON schema #1404

Merged
merged 10 commits into from
Jan 20, 2022
Merged

Add DatasetLinks to JSON schema #1404

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9e2a551
first try
sappelhoff Jan 16, 2022
cee279a
add functional DatasetLinks schema: any str bigger than len 0 --> uri
sappelhoff Jan 16, 2022
56cadbd
add tests
sappelhoff Jan 16, 2022
f01b2cc
fix lint
sappelhoff Jan 16, 2022
79df004
use propertyNames feature, instead of not feature
sappelhoff Jan 19, 2022
fecc99c
migrated eeg_matchingpennies from GitHub to GIN
sappelhoff Jan 19, 2022
03fb0eb
fix formatting E501
sappelhoff Jan 19, 2022
08b4431
Update bids-validator/tests/json.spec.js
sappelhoff Jan 19, 2022
cfa249e
Update bids-validator/validators/json/schemas/dataset_description.json
effigies Jan 19, 2022
a7e4bae
fix tests
sappelhoff Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bids-validator/bids_validator/test_bids_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
HOME = os.path.expanduser('~')

TEST_DATA_DICT = {
'eeg_matchingpennies': 'https://github.com/sappelhoff/eeg_matchingpennies'
'eeg_matchingpennies': (
'https://gin.g-node.org/sappelhoff/eeg_matchingpennies'
),
}

EXCLUDE_KEYWORDS = ['git', 'datalad', 'sourcedata', 'bidsignore']
Expand Down
74 changes: 74 additions & 0 deletions bids-validator/tests/json.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,80 @@ describe('JSON', function() {
relativePath: '/dataset_description.json',
}

it('dataset_description.json should validate DatasetLinks', function() {
var jsonObj = {
Name: 'Example Name',
BIDSVersion: '1.4.0',
DatasetLinks: {
mylink: 'https://www.google.com',
sappelhoff marked this conversation as resolved.
Show resolved Hide resolved
},
}
jsonDict[dataset_description_file.relativePath] = jsonObj
validate.JSON(dataset_description_file, jsonDict, function(issues) {
assert(issues.length === 0)
})
})

it('dataset_description.json should raise on empty key in DatasetLinks', function() {
var jsonObj = {
Name: 'Example Name',
BIDSVersion: '1.4.0',
DatasetLinks: {
mylink: 'https://www.google.com',
'': 'https://www.yahoo.com',
},
}
jsonDict[dataset_description_file.relativePath] = jsonObj
validate.JSON(dataset_description_file, jsonDict, function(issues) {
assert(issues.length === 2)
assert(
issues[0].evidence ==
'.DatasetLinks should NOT be shorter than 1 characters',
)
assert(issues[1].evidence == ".DatasetLinks property name '' is invalid")
})
})

it('dataset_description.json should raise on wrong key in DatasetLinks', function() {
var jsonObj = {
Name: 'Example Name',
BIDSVersion: '1.4.0',
DatasetLinks: 'https://www.google.com',
}
jsonDict[dataset_description_file.relativePath] = jsonObj
validate.JSON(dataset_description_file, jsonDict, function(issues) {
assert(issues.length === 1)
assert(issues[0].evidence == '.DatasetLinks should be object')
})
})

it('dataset_description.json should raise on invalid values in DatasetLinks', function() {
var jsonObj = {
Name: 'Example Name',
BIDSVersion: '1.4.0',
DatasetLinks: {
mylink1: 'https://www.google.com',
mylink2: 1,
mylink3: 'nope',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is now valid. Can either drop it or make it invalid.

Suggested change
mylink3: 'nope',
mylink3: ':/nope',

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'll drop it and change it to a test that fails for keys like I am a key with whitespace and not alphanumeric "§%

'': 'https://www.yahoo.com',
},
}
jsonDict[dataset_description_file.relativePath] = jsonObj
validate.JSON(dataset_description_file, jsonDict, function(issues) {
assert(issues.length === 4)
assert(
issues[0].evidence ==
'.DatasetLinks should NOT be shorter than 1 characters',
)
assert(issues[1].evidence == ".DatasetLinks property name '' is invalid")
assert(issues[2].evidence == ".DatasetLinks['mylink2'] should be string")
assert(
issues[3].evidence ==
'.DatasetLinks[\'mylink3\'] should match format "uri"',
)
})
})

it('dataset_description.json should validate with enum of DatasetType', function() {
var jsonObj = {
Name: 'Example Name',
Expand Down
13 changes: 13 additions & 0 deletions bids-validator/validators/json/schemas/dataset_description.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,19 @@
"DatasetDOI": {
"type": "string"
},
"DatasetLinks": {
"type": "object",
"properties": { },
"propertyNames": {
"type": "string",
"minLength": 1,
"pattern": "^[a-zA-Z0-9]*$"
Comment on lines +60 to +61
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about:

Suggested change
"minLength": 1,
"pattern": "^[a-zA-Z0-9]*$"
"pattern": "^[a-zA-Z0-9]+$"

Or would that make the error less clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that was my train of thought: better to be explicit here. Everyone can guess what minLength means, but few people know the difference between * and + in a regex

Copy link
Member Author

Choose a reason for hiding this comment

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

don't know about the error message though (on re-reading, that seems to be your concern, whereas mine was with future devs)

Copy link
Member Author

Choose a reason for hiding this comment

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

it would change .DatasetLinks should NOT be shorter than 1 characters to '.DatasetLinks should match pattern "^[a-zA-Z0-9]+$"'

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern is that we get both:

'.DatasetLinks should NOT be shorter than 1 characters'
".DatasetLinks property name '' is invalid"

The second one seems more helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

My concern is that we get both

I see what you mean, but that's also the case after your suggested fix unfortunately. It'd just be

'.DatasetLinks should match pattern "^[a-zA-Z0-9]+$"'
".DatasetLinks property name '' is invalid"

instead.

I think both of these error messages are helpful though (much more helpful than before in any case, so thanks for your feedback)

},
"additionalProperties": {
"type": "string",
"format": "uri"
effigies marked this conversation as resolved.
Show resolved Hide resolved
}
},
"GeneratedBy": {
"type": "array",
"minItems": 1,
Expand Down