pelias / model

Pelias data models
6 stars 17 forks source link

Adding parentFields validation in addParent. #150

Closed mansoor-sajjad closed 2 years ago

mansoor-sajjad commented 2 years ago

:wave:


Here's the reason for this change :rocket:

I need to update the pelias-csv-importer to support the import of parent field. And found that the addParent function does not validate the field name with the list of parentFields, like it is done with the addressFields in setAddress function.


Here's what actually got changed :clap:

Updated the addParent to function by adding the property validation for parentFields. Added the unit test to test the change.

missinglink commented 2 years ago

Hi @mansoor-sajjad looking over the list of supported parent fields in pelias/schema there seems to be a difference from the list in this repo 🤔

It's a little tricky to compare since the order is different but I think 'macrohood' is listed here but not supported in the schema, could you please double-check?

mansoor-sajjad commented 2 years ago

Hi, @missinglink , nice catch. I have double checked with pelias-schema, macrohood is missing there in the schema. How do we fix it? By adding a support in the schema or by removing from the model? I guess the latter. 🤔

missinglink commented 2 years ago

Please remove it from model as part of this PR

mansoor-sajjad commented 2 years ago

@missinglink What is the further process to get that PR merged?

missinglink commented 2 years ago

I pulled your code locally and the unit tests showed as failing.

There's a weird issue with Github Actions where it doesn't run the CI for people outside of our org. I've written https://github.com/pelias/model/pull/151 in an attempt to fix it, can you please rebase master against your branch and push to see if the CI runs?

mansoor-sajjad commented 2 years ago

Hi @missinglink ,

Sorry for the late response, I was actually on vacations. I have now rebased this branch with changes you have made to run the unit tests on pull-requests. I have also fixed the failing unit tests. Can you please have a quick look at it?

missinglink commented 2 years ago

Thanks for your contribution, this patch was released as pelias-model@9.4.0 on npm.