-
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
F/3978 customizable spatial attribute #25
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.
Great work Mark! Very thorough and understandable. I left a couple minor comments about tests, but other than that you are good to go!
@@ -944,7 +942,7 @@ describe('formatDcatDataset', () => { | |||
} | |||
}); | |||
|
|||
it('overwrites critical fields without values to an empty string', () => { | |||
it('does not define spatial property when template does not specify it', () => { |
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 looks like you may have deleted the generic overwrites critical fields
test instead of just adding tests for spatial
. If that's the case, would you mind adding it back in?
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 turns out that once you remove spatial
there are no longer any, at least from what I can tell, non-editable fields that are at the dataset-level, instead they are all feed-level or will never get populated by the dataset (contactPoint[@type]). If this is incorrect i can add back in pretty easily
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.
Sorry for the false alarm, you're right. The git diff was a little tricky so I thought that more was in that test.
fail('Should not throw!'); | ||
} | ||
const formatted = JSON.parse(formatDcatDataset(dataset, siteUrl, siteModel, buildDatasetTemplate({ spatial: undefined }))); | ||
expect(formatted.hasOwnProperty('spatial')).toBeFalsy(); |
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.
The test title says does not define spatial property
, so perhaps we should check with .toBeUndefined()
instead of .toBeFalsy()
to be more explicit.
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.
And if this is something you change, we'll want to change it in all the other tests too
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.
In this case I am actually being more specific.
The test should fail if spatial is ever returned with an undefined
value. In that case, expect(format.spatial).toBeUndefined()
does not actually assert what we want, as { spatial: undefined }
would pass the test when it should fail. Instead, each dataset should not have the property at all, hence why I went with hasOwnProperty.
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.
🤦♂️ Sorry, the hasOwnProperty
didn't register in my mind until you pointed it out. You are good to go.
This PR:
extent
oritemExtent
fields. The returned string is always 4 numbers fixed to 4 decimals each