Use array path when delimiter is false#29
Use array path when delimiter is false#29grey-stripe wants to merge 2 commits intoliufengyun:masterfrom
Conversation
|
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? |
|
@liufengyun - thanks for taking a look! I removed the Let me know if you'd like any other changes made. |
liufengyun
left a comment
liufengyun
left a comment
There was a problem hiding this comment.
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.
| # | ||
| # decode property path into an array | ||
| # decode a property path into an array | ||
| # @param [String] path Property-string |
There was a problem hiding this comment.
I'm fine to remove this as we are moving to array representation, this is no longer relevant.
| # @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 | ||
| # |
There was a problem hiding this comment.
I think we can just remove the delimiter and break compatibility.
|
|
||
| opts[:prefix] = "#{opts[:prefix]}[*]" | ||
| def self.lcs(a, b, options) | ||
| options = options.merge({:prefix => options[:prefix] + [-1]}) |
There was a problem hiding this comment.
Could you add a comment here why plus -1?
| # @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 | ||
| # |
There was a problem hiding this comment.
I think it's fine just to remove delimiter.
2 participants
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:
:delimiteroption now takesfalse, in which case paths are returned as arrays rather than strings (Key that includes delimiter errors on patch! #19).:stringify_keys, defaulting totrue.false, object keys will not be converted to strings in array paths.:delimiteris false.patch!andunpatch!work automatically with array paths, no option necessary.README.mddescription of new option values.Internal changes:
:prefixis now maintained in all relevant functions (most notably missing wasdiff_array).:delimiteris in effect, these paths will be translated to string paths at the API boundaries.'[*]'in string paths is now-1in array paths (only exposed in comparison callbacks).diff_objectout ofdiff, similarly todiff_array.diffis now a wrapper fordiff_internaldiff_internal.