-
Notifications
You must be signed in to change notification settings - Fork 0
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
b/2531 fix license resolution service url and missing extent/theme #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments I'd like resolved before we merge, but thank you for taking this on!
@@ -342,7 +342,7 @@ describe('formatDcatDataset', () => { | |||
const expected = { | |||
'@type': 'dcat:Dataset', | |||
identifier: `${siteUrl}/datasets/00000000000000000000000000000000_0`, | |||
license: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drspacemanphd I changed license to be null instead of '' for an issue a couple months ago. Are we sure we want to go back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing, the switch to null was a style preference on Tate's part, not a product requirement. I'm fine to leave this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really a style thing on my part. It all comes down to the DCAT harvesters. Thomas told me that they seemed to prefer null
, so I tried to be consistent with that. If it doesn't matter, this is fine with me!
Another thing to be aware of is that when you have a JSON-LD node, sometimes the entire node should be removed when one of the values isn't available.
E.g.
"foo": {
"@id": "dct:somekey",
"importantValue": null
}
I think adlib
can do this by means of the "Optional Transform.".
@@ -12,5 +12,6 @@ export const baseDatasetTemplate = { | |||
fn: '{{owner}}', | |||
hasEmail: '{{orgContactEmail:optional}}' | |||
}, | |||
accessLevel: 'public' | |||
accessLevel: 'public', | |||
spatial: '{{extent}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the theme
property mentioned in the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this comes along with extent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
2531 - Fixes licensing resolution, and adds the correct geoservice url and spatial/theme properties