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

Handle multi dimentional arrays in path decoder #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmitrytrager
Copy link

This fixes issue related to handling multi dimensional arrays in path

@liufengyun
Copy link
Owner

Thanks @dmitrytrager . What's your use case? Multi-dimentional arrays will not work nicely with this library due to its limitations.

@dmitrytrager
Copy link
Author

My case is exactly like mentioned: nested arrays within hash.
With this fix we can handle changes within structure like this properly.

@liufengyun
Copy link
Owner

I see. But generally, the diff generated by multi-dimensional arrays will be very bad to be useful.

@dmitrytrager
Copy link
Author

Could you please give more details about reasons here?
B/c right now it looks like if my structure includes these, then diff should be very useful.

@liufengyun
Copy link
Owner

Could you please give more details about reasons here?

The reason is that telling whether two arrays are similar may not make much sense in most scenarios.

@dmitrytrager
Copy link
Author

Sorry, I don't get how does less frequent scenario mean "very bad to be useful"

@liufengyun
Copy link
Owner

liufengyun commented Sep 16, 2022

Sorry, I don't get how does less frequent scenario mean "very bad to be useful"

I don't have objective data to tell whether it's generally useful or not. I just want to avoid disappointing the users with a feature that is not well-considered.

Anyway, thank you for making the PR and making the use case known. Let's wait for more users to share their feedback on this feature.

@dmitrytrager
Copy link
Author

Ok, having feedback would be great

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