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.

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

Adding parentFields validation in addParent. #150

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

mansoor-sajjad
Copy link
Contributor

👋


Here's the reason for this change 🚀

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 👏

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

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Looks good to me, did it fix your issue?

@missinglink
Copy link
Member

missinglink commented Jun 16, 2022

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
Copy link
Contributor Author

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
Copy link
Member

Please remove it from model as part of this PR

@mansoor-sajjad
Copy link
Contributor Author

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

@missinglink
Copy link
Member

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 #151 in an attempt to fix it, can you please rebase master against your branch and push to see if the CI runs?

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Please resolve the unit test failure

@mansoor-sajjad
Copy link
Contributor Author

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 missinglink merged commit 83b64de into pelias:master Aug 1, 2022
@missinglink
Copy link
Member

missinglink commented Aug 1, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants