parse geojson from provider #32
Conversation
sansth1010
requested review from
velveetachef,
rgwozdz,
ethanspiro,
sonofflynn89 and
drspacemanphd
src/dcat-us/compile-dcat-feed.ts
Outdated
| {}, | ||
| defaultDataset, | ||
| adlib(feedTemplate, { ...dataset?.properties, ...dataset?.geometry }, feedTemplateTransforms) | ||
| ); |
There was a problem hiding this comment.
Too much going on here to easily understand. Can you move to a helper and make it more clear?
There was a problem hiding this comment.
Agreed! Moved it into generateDcatItem() function and updated variable names to make it more clear.
src/dcat-us/compile-dcat-feed.ts
Outdated
|
|
||
| // moving feature properties fields at the top level | ||
| // and geometry to second as the values for the properties | ||
| // can be easily referenced in template |
There was a problem hiding this comment.
Comments here are not necessary. They should only be used when the code cannot be written in away that explains itself. See https://bpoplauschi.github.io/2021/01/20/Clean-Code-Comments-by-Uncle-Bob-part-2.html.
There was a problem hiding this comment.
That is great point. I wanted to mention why we are restructuring geojson properties and geometry like I am doing right now as opposed to passing the whole object geojsonFeature. This would relate to how we are mapping the values in the template. But, I have removed the comment.
src/dcat-us/compile-dcat-feed.ts
Outdated
| // and geometry to second as the values for the properties | ||
| // can be easily referenced in template | ||
| const dcatFeedData = { | ||
| ...geojsonFeature?.properties, |
There was a problem hiding this comment.
I think you don't need the ? as geojson should always have properties. But if you are worried about it not being defined, I think this code will cause an exception. If properties is undefined, it will try to destructure undefined and you'll have an error.
I think this is probably not under test. I think you should implement test coverage, and add tests for whatever is not covered.
There was a problem hiding this comment.
That is true. My intention was to not allow it to fail even if some if some of geojson attributes are missing. I have removed the optional chaining operator for now and added test coverage for it.
src/dcat-us/compile-dcat-feed.ts
Outdated
| // can be easily referenced in template | ||
| const dcatFeedData = { | ||
| ...geojsonFeature?.properties, | ||
| ...{ geometry: geojsonFeature?.geometry } |
There was a problem hiding this comment.
I think this destructuring is unnecessary. Can't you just do:
geometry: geojsonFeature.geometry
There was a problem hiding this comment.
Yes, we can remove the unnecessary destructuring.
src/dcat-us/compile-dcat-feed.ts
Outdated
| @@ -25,6 +22,32 @@ function removeUninterpolatedDistributions(distributions: any[]) { | |||
| return distributions.filter((distro) => !(typeof distro === 'string' && distro.match(/{{.+}}/)?.length)); | |||
| } | |||
|
|
|||
| function generateDcatItem(feedTemplate, feedTemplateTransforms, geojsonFeature) { | |||
There was a problem hiding this comment.
this is typescript, but you have not given the parameters types, and the function doesn't have a return type.
There was a problem hiding this comment.
Yes, missed the types. I had added them. However. let me know if I need to narrow down my types to be even more concrete.
|
@rgwozdz : Test coverage currently:
There were few trivial unreachable conditions from previous code that needed to updated. |
PR for 5620