Skip to content

Add rootParentIds option to fit the case when all items contain a valid parentId string #24

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

Merged
merged 5 commits into from
Feb 2, 2021

Conversation

rrbe
Copy link
Contributor

@rrbe rrbe commented Aug 12, 2020

See #23

Copy link
Owner

@philipstanislaus philipstanislaus left a comment

Choose a reason for hiding this comment

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

Thanks, left a few comments.

I feel that we should not merge the supplied array with the default, but replace it. Otherwise there is no fine grained control when to throw. What do you think?

@rrbe
Copy link
Contributor Author

rrbe commented Aug 13, 2020

Thanks, left a few comments.

I feel that we should not merge the supplied array with the default, but replace it. Otherwise there is no fine grained control when to throw. What do you think?

These modifications are completed, and I also added test case about the rootParentId options replacement, please check it and see what else needs to be modified

Copy link
Owner

@philipstanislaus philipstanislaus left a comment

Choose a reason for hiding this comment

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

Thanks, just one more note, please see the comments.

Copy link
Owner

@philipstanislaus philipstanislaus left a comment

Choose a reason for hiding this comment

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

@rrbe sorry for the very late feedback, I just remembered there were some issues/PR still open here.

I have thought about the architecture a bit again and changed the data type for rootParentIds to an object for better performance.

This should be fine now – I will merge it shortly.

Many thanks for your contribution and sorry again for the delay!

@philipstanislaus philipstanislaus merged commit 4fe9dc2 into philipstanislaus:master Feb 2, 2021
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