✨ Parse list notation #159

nlepage · 2017-12-08T09:52:10Z

Issue : fix #151

codecov-io · 2017-12-11T10:21:40Z

Codecov Report

Merging #159 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #159   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          79     79           
  Lines         296    306   +10     
=====================================
+ Hits          296    306   +10

Impacted Files	Coverage Δ
packages/immutadot/src/path/consts.js	`100% <100%> (ø)`	⬆️
packages/immutadot/src/path/toPath.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20a1153...70eb1d6. Read the comment docs.

nlepage · 2017-12-11T10:51:00Z

I deliberatly changed the behavior of incomplete notation for the list.
Instead of running to the end of the path like array and slice do, we make a prop including the opening curly until the next segment.
I think most of the time this will give a result closer to what the user meant, and highlight his mistake with the opening curly included in the prop.

@hgwood @frinyvonnick What do you think ? If you are OK, I will comment on #113 in order to refactor array and slice to make the same.

I also mutualized the later parsers in only one.
I had not anticipated the removal of str => [[prop, str]], but code coverage reported that this code was dead, and all the tests were still working ! The perks of having 100% coverage !

hgwood · 2017-12-11T10:48:42Z

packages/immutadot/src/path/toPath.js

-  ([beforeBracket, atBracket]) => [[prop, beforeBracket], ...stringToPath(atBracket)],
+const pathSegmentEndedByNewSegment = map(
+  regexp(/^([^.[{]*)\.?([[{]?.*)$/),
+  ([beforeNewSegment, rest]) => [[prop, beforeNewSegment], ...stringToPath(rest)],


Nice simplification!

hgwood · 2017-12-11T10:49:49Z

packages/immutadot/src/path/toPath.js

+  regexp(/^(\{[^.[{]*)(.*)$/),
+  ([beforeNewSegment, rest]) => {
+    return [[prop, beforeNewSegment], ...stringToPath(rest)]
+  },


Maybe the lambda could be written in the expression form (ie without braces and return)?

hgwood · 2017-12-11T10:50:57Z

packages/immutadot/src/path/toPath.js

+    for (let propMatch = rawProps.match(listPropRegexp); propMatch !== undefined; propMatch = propMatch[5] === '' ? undefined : propMatch[5].match(listPropRegexp))
+      props.push(propMatch[2] === undefined ? propMatch[4] : propMatch[2])
+    return props.length === 1 ? [[prop, props[0]], ...stringToPath(rest)] : [[list, props], ...stringToPath(rest)]
+  },


This piece of code is a little overwhelming. My advice would be:

use destructuring to name the elements of propMatch

use functional programming to make meaningful transformation names emerge (what is this loop doing? a reduction? a mapping?)

hgwood · 2017-12-11T10:54:58Z

packages/immutadot/src/path/toPath.js

-  ([beforeDot, afterDot]) => [[prop, beforeDot], ...stringToPath(afterDot)],
+const listPropRegexp = /^,?((?!["'])([^,]*)|(["'])(.*?[^\\])\3)(.*)/
+const listNotationParser = map(
+  regexp(/^\{(((?!["'])[^,}]*|(["']).*?[^\\]\2)(,((?!["'])[^,}]*|(["']).*?[^\\]\6))*)\}\.?(.*)$/),


We might want to split the quoted case and the bare case like I did for the bracket notation. I feel it yielded simpler regexes. These ones are a bit hard to read.

hgwood · 2017-12-11T11:03:13Z

@nlepage *their mistake :)

That last parser is dead because pathSegmentEndedByNewSegment already matches everything. Again, nice simplification.

As for the incomplete cases, I'm fine both ways.

nlepage · 2017-12-11T22:07:02Z

@hgwood I've used a generator to extract list props, and propMatch elements are now named as you suggested, what do you think ?
Regarding the main regexp, I don't see how to split quoted and bare case... Do you have any pointers ?

frinyvonnick · 2017-12-12T09:05:08Z

@nlepage i agree for incomplete cases !

hgwood · 2017-12-12T13:55:09Z

@nlepage Great improvement to listNotationParser, I think it is a lot faster to read and understand now.

As for splitting the regex, I see now that you cannot really do that as you may have lists with both quoted and unquoted props. I'm still a bit worried by the length of the regex and also the duplication. I think I may have an idea to mitigate that, but I don't want to block this PR on it.

My idea is to go full parser combinators to have modular, composable, easy-to-understand parsers so you could describe lists like between("{}", interspersedBy(",", maybeBetween("''\"\""))). Might be an interesting refactoring to undertake, but again, not worth blocking on.

charlyx

👌

frinyvonnick · 2017-12-12T20:51:02Z

packages/immutadot/src/path/toPath.js

-  ([beforeDot, afterDot]) => [[prop, beforeDot], ...stringToPath(afterDot)],
+const listPropRegexp = /^,?((?!["'])([^,]*)|(["'])(.*?[^\\])\3)(.*)/
+function* extractListProps(rawProps) {
+  if (rawProps.startsWith(',')) yield ''


This case isn't tested 😢

frinyvonnick · 2017-12-12T20:55:19Z

packages/immutadot/src/path/toPath.js

+const listNotationParser = map(
+  regexp(/^\{(((?!["'])[^,}]*|(["']).*?[^\\]\2)(,((?!["'])[^,}]*|(["']).*?[^\\]\6))*)\}\.?(.*)$/),
+  ([rawProps, , , , , , rest]) => {
+    const props = [...extractListProps(rawProps)]


Smart 💡 !

frinyvonnick · 2017-12-12T21:02:06Z

Nice work 👍

nlepage added the ✨ feature label Dec 8, 2017

nlepage added this to the 1.0.0-RC1 milestone Dec 8, 2017

nlepage requested a review from frinyvonnick as a code owner December 8, 2017 09:52

nlepage self-assigned this Dec 8, 2017

nlepage force-pushed the feature/151 branch from 24cbd8b to 4e4cbe0 Compare December 8, 2017 15:14

nlepage changed the title ~~🚧 Parse list notation~~ ✨ Parse list notation Dec 11, 2017

nlepage requested a review from hgwood December 11, 2017 10:39

hgwood reviewed Dec 11, 2017

View reviewed changes

nlepage requested a review from charlyx December 11, 2017 22:07

nlepage added 10 commits December 12, 2017 10:10

✅ Update toPath tests for list notation

092a96e

✅ Add tests for unterminated list notation

cb35d02

🚧 Parse bare list notation

e2cae7a

♻ Rewrite bareListNotationParser

d72d48d

🚧 Manage incomplete bare list notation

5595e26

✨ List notation parser !

7e4f2d3

♻️ Mutualize some parsers

01fe63f

🔥 Remove dead code

ab00ac7

👌 Review @hgwood

e332c46

👌 review @hgwood, extract list props with a generator

ebd91b1

nlepage force-pushed the feature/151 branch from bf545e4 to ebd91b1 Compare December 12, 2017 09:14

nlepage mentioned this pull request Dec 12, 2017

toPath consistency #113

Closed

2 tasks

hgwood previously approved these changes Dec 12, 2017

View reviewed changes

charlyx previously approved these changes Dec 12, 2017

View reviewed changes

frinyvonnick suggested changes Dec 12, 2017

View reviewed changes

👌 review @frinyvonnick

70eb1d6

nlepage dismissed stale reviews from charlyx and hgwood via 70eb1d6 December 12, 2017 21:00

frinyvonnick approved these changes Dec 12, 2017

View reviewed changes

nlepage merged commit 90e27d8 into master Dec 12, 2017

nlepage deleted the feature/151 branch December 12, 2017 21:03

This was referenced Dec 14, 2017

Path syntax: wildcard for all properties #166

Closed

Path syntax: merge slice syntax and prop list syntax #167

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Parse list notation #159

✨ Parse list notation #159

nlepage commented Dec 8, 2017

codecov-io commented Dec 11, 2017 •

edited

nlepage commented Dec 11, 2017

hgwood Dec 11, 2017

hgwood Dec 11, 2017

hgwood Dec 11, 2017

hgwood Dec 11, 2017

hgwood commented Dec 11, 2017

nlepage commented Dec 11, 2017

frinyvonnick commented Dec 12, 2017

hgwood commented Dec 12, 2017 •

edited

charlyx left a comment

frinyvonnick Dec 12, 2017

frinyvonnick Dec 12, 2017 •

edited

frinyvonnick commented Dec 12, 2017

✨ Parse list notation #159

✨ Parse list notation #159

Conversation

nlepage commented Dec 8, 2017

codecov-io commented Dec 11, 2017 • edited

Codecov Report

nlepage commented Dec 11, 2017

hgwood Dec 11, 2017

Choose a reason for hiding this comment

hgwood Dec 11, 2017

Choose a reason for hiding this comment

hgwood Dec 11, 2017

Choose a reason for hiding this comment

hgwood Dec 11, 2017

Choose a reason for hiding this comment

hgwood commented Dec 11, 2017

nlepage commented Dec 11, 2017

frinyvonnick commented Dec 12, 2017

hgwood commented Dec 12, 2017 • edited

charlyx left a comment

Choose a reason for hiding this comment

frinyvonnick Dec 12, 2017

Choose a reason for hiding this comment

frinyvonnick Dec 12, 2017 • edited

Choose a reason for hiding this comment

frinyvonnick commented Dec 12, 2017

codecov-io commented Dec 11, 2017 •

edited

hgwood commented Dec 12, 2017 •

edited

frinyvonnick Dec 12, 2017 •

edited