Use array path when delimiter is false #29

grey-stripe · 2017-01-11T01:39:37Z

Addresses #19 and #25.

I went through some effort to make this backwards compatible - no external-facing behavior should be different using default options. This could be made simpler by breaking the default behavior, so let me know which you would prefer.

User-facing changes:

The :delimiter option now takes false, in which case paths are returned as arrays rather than strings (Key that includes delimiter errors on patch! #19).
Added a new option :stringify_keys, defaulting to true.
- When false, object keys will not be converted to strings in array paths.
- This should allow for diffing of objects where symbol keys have meaning (Patch problem whit symbol as key #25).
- This only applies if :delimiter is false.
patch! and unpatch! work automatically with array paths, no option necessary.
Added README.md description of new option values.

Internal changes:

:prefix is now maintained in all relevant functions (most notably missing was diff_array).
- When a change is added to the result set, it contains the fully qualified path and does not need an extra step to finalize it.
Paths are now maintained internally as array paths rather than string paths.
- When :delimiter is in effect, these paths will be translated to string paths at the API boundaries.
- '[*]' in string paths is now -1 in array paths (only exposed in comparison callbacks).
Broke diff_object out of diff, similarly to diff_array.
diff is now a wrapper for diff_internal
- Primarily, this was to provide the API boundary for translating back to string paths.
- Option default values are now applied once here, not every recursive call to diff_internal.
Removed several default values on options (most weren't used except in tests).
- Let me know if this is too intrusive, I may have gotten carried away.
Added tests for the new behavior.

liufengyun · 2017-02-21T03:36:04Z

Thanks @grey-stripe , it looks great 👍 Sorry I somehow missed the PR notification.

You mentioned we it could be simpler by breaking the default behavior. I think I'm now happy to make a major release to break backward-compatibility, and make array diff the default. Could you update the PR?

grey-stripe · 2017-02-23T00:17:58Z

@liufengyun - thanks for taking a look! I removed the delimiter option so it no longer needs to translate array paths to string paths at the library boundaries. I left patch backwards compatible because it did not complicate any of the rest of the implementation, and (at least in my case) I have old patch diffs stored, and would like them to keep working when I upgrade.

Let me know if you'd like any other changes made.

liufengyun

Thanks @grey-stripe, this is awesome 🍷 !

In addition to some comments in the code, I have two more comments below.

First, a lot of examples and contents in README are irrelevant with this PR. Could you please also update or remove obsolete contents in README?

Second, I see now an implicit design decision is to just represent array change as an integer in the prefix. I think this is an important knowledge for people using the library. I think it's worth documenting it in the README and potential problems with it.

Otherwise, it looks good to me.

liufengyun · 2017-02-23T05:37:10Z

lib/hashdiff/util.rb

@@ -48,12 +48,12 @@ def self.count_nodes(obj)

 # @private
 #
- # decode property path into an array
+ # decode a property path into an array
 # @param [String] path Property-string


I'm fine to remove this as we are moving to array representation, this is no longer relevant.

liufengyun · 2017-02-23T05:38:34Z

lib/hashdiff/patch.rb

 # @param [Hash] options supports following keys:
- # * :delimiter (String) ['.'] delimiter string for representing nested keys in changes array
+ # * :delimiter (String) ['.'] delimiter string for representing nested keys in changes array, ignored for array paths
 #


I think we can just remove the delimiter and break compatibility.

liufengyun · 2017-02-23T05:41:21Z

lib/hashdiff/lcs.rb

-
- opts[:prefix] = "#{opts[:prefix]}[*]"
+ def self.lcs(a, b, options)
+ options = options.merge({:prefix => options[:prefix] + [-1]})


Could you add a comment here why plus -1?

liufengyun · 2017-02-23T05:42:13Z

lib/hashdiff/patch.rb

 # @param [Hash] options supports following keys:
- # * :delimiter (String) ['.'] delimiter string for representing nested keys in changes array
+ # * :delimiter (String) ['.'] delimiter string for representing nested keys in changes array, ignored for array paths
 #


I think it's fine just to remove delimiter.

use array path when delimiter is false

69efcf6

remove backwards compatibility of array path changes

2bbcc68

liufengyun reviewed Feb 23, 2017

View reviewed changes

liufengyun mentioned this pull request Jul 29, 2017

Introduce an array_path option #34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use array path when delimiter is false #29

Use array path when delimiter is false #29

grey-stripe commented Jan 11, 2017 •

edited

Loading

liufengyun commented Feb 21, 2017

grey-stripe commented Feb 23, 2017 •

edited

Loading

liufengyun left a comment

liufengyun Feb 23, 2017

liufengyun Feb 23, 2017

liufengyun Feb 23, 2017

liufengyun Feb 23, 2017

Use array path when delimiter is false #29

Are you sure you want to change the base?

Use array path when delimiter is false #29

Conversation

grey-stripe commented Jan 11, 2017 • edited Loading

liufengyun commented Feb 21, 2017

grey-stripe commented Feb 23, 2017 • edited Loading

liufengyun left a comment

Choose a reason for hiding this comment

liufengyun Feb 23, 2017

Choose a reason for hiding this comment

liufengyun Feb 23, 2017

Choose a reason for hiding this comment

liufengyun Feb 23, 2017

Choose a reason for hiding this comment

liufengyun Feb 23, 2017

Choose a reason for hiding this comment

grey-stripe commented Jan 11, 2017 •

edited

Loading

grey-stripe commented Feb 23, 2017 •

edited

Loading