add allowDotsForNumbers option to convert from brackets of indices to dots#542
add allowDotsForNumbers option to convert from brackets of indices to dots#542antareepsarkar wants to merge 3 commits intoljharb:mainfrom
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, just forgot about parse. I will modify that as well. |
ljharb
left a comment
There was a problem hiding this comment.
Note that the issue suggests "allowDotsForNumbers" - "for indices" and "for numbers" are distinctly different and imply different semantics.
Additionally, the intention was for allowDots to be required to be explicitly passed as true when the new option is passed.
Also, if this new option is omitted, behavior MUST not change, and in this PR, it does.
If allowDotsForIndices: true and encodeDotInKeys: true, the dots used for indices would get encoded as %2E, defeating the purpose. There's no guard for this.
lib/parse.js
Outdated
| var splitKeyIntoSegments = function splitKeyIntoSegments(givenKey, options) { | ||
| var key = options.allowDots ? givenKey.replace(/\.([^.[]+)/g, '[$1]') : givenKey; | ||
| var key = options.allowDots | ||
| ? options.allowDotsForIndices ? givenKey.replace(/\.([^.[]+)/g, '[$1]') : givenKey.replace(/\.(?!\d)([^.[\]]+)/g, '[$1]') : givenKey; |
There was a problem hiding this comment.
this regex differs from the original in the capture group: it adds ] to the exclusion set ([^.[]] vs [^.[). This means keys containing ] that were previously captured now won't be. This would be a breaking change.
additionally, the negative lookahead (?!\d) is too broad. It prevents ANY dot-before-digit from being treated as property access, not just dots before valid array indices. For example, a.1foo.b=c - 1foo is not a valid array index, but the regex would still skip .1foo, producing { 'a.1foo': { b: 'c' } } instead of the current { a: { '1foo': { b: 'c' } } }, which is another breaking change.
resolves #516
allowDotsdoes not convert bracket notation for array indices to dots.This PR adds an option
allowDotsForIndiceswhich does that.