Rewrite parser in TS

[nlepage]

@hgwood

[frinyvonnick]

Why do you use " and ; ?

[nlepage]

Why do you use " and ; ?

I used default tslint config

[hgwood]

OK I got around to investigating this. The code does not actually compile 😅, but since the config file is in a non-standard location, VS Code does not pick it up and there's no editor feedback. npm run build on the other hand gives the errors as expected.

[hgwood]

I think you'll hit one major barrier. It seems that all parsers can return either a result or null, except for one, applyParsers, which the code assumes never returns null. This assumption is materialized by the code using the spread operator on the result of this parser without checking for null.

I guess the code can make this assumption because all of the sub-parsers employed in applyParsers make for an exhaustive parser (all possibilities are accounted for), though I'm really not sure about this, it might be hard to prove? Anyway it seems that the code does not handle the case where applyParsers returns null, and the new type system now forces it to handle it, even if it's a potentially impossible case.

I see two courses of action. The easy approach would be to handle the case in applyParsers itself (maybe by throwing an error). The hard but more elegant and ambitious (if at all possible) approach is to prove that applyParsers is exhaustive find a way to encode this proof in the types.

[nlepage]

@hgwood Thx for the inputs, I fixed the build errors, unit tests are OK, feel free to play around with the branch.

[hgwood]

Last commit open for review: https://github.com/Zenika/immutadot/pull/327/commits/11f2b2f61a5dbb2066322ca1182d0a3f66eb880f

• made the Parser type generic
• split the "TypeError if parser returns null" code into its own function succeedOrThrow
• added path segments constructors to ensure that segments are always properly typed
• moved all slice index code to utils
• moved all path parsing code to toPath

I'd like to go a bit further with a Elm-like (type-centered) file structure.

[nlepage]

I'll review new commits tomorrow. We have to rebase this work on master, I'm discarding the dev branch.

[nlepage]

LGTM! The new type centric arrangement feels nice. Pushed a few minor changes.

[nlepage]

andThen and ignorePrefix make this test fail:

    expect(toPath(".")).toEqual([
      propSegment(""),
    ]);

with:

    Expected value to equal:
      [["prop", ""]]
    Received:
      []

is this intentional ?

[hgwood]

@nlepage no, not intentional. I simply misread the old code and introduced a bug. I'll have a look but it might be just a question of changing the order of application of ignorePrefix and andThen.

[hgwood]

@nlepage I'm having trouble running the test suite but my last commit fixes the particular case that was broken. Hopefully it does not break another case.

[nlepage]

@hgwood :ok_hand: All tests OK for me

[nlepage]

Works :tada:

Used esm to load es2015 parser in node !

[codecov-io]

Codecov Report

Merging #327 into master will increase coverage by 0.24%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   97.08%   97.32%   +0.24%     
==========================================
  Files         107      110       +3     
  Lines         377      411      +34     
  Branches        0       11      +11     
==========================================
+ Hits          366      400      +34     
  Misses         11       11

Impacted Files	Coverage Δ
packages/immutadot-parser/src/maybe.ts	100% <100%> (ø)
packages/immutadot-parser/src/sliceIndex.ts	100% <100%> (ø)
packages/immutadot/src/nav/nav.js	87.5% <100%> (ø)	:arrow_up:
packages/immutadot-parser/src/utils.ts	100% <100%> (ø)
packages/immutadot-parser/src/pathParser.ts	100% <100%> (ø)
packages/immutadot-parser/src/path.ts	100% <100%> (ø)
packages/immutadot-parser/src/parser.ts	100% <100%> (ø)
packages/immutadot-parser/src/toPath.ts	100% <100%> (ø)
... and 5 more

---

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 551e60d...ddec3b3. Read the comment docs.

[nlepage]

Rebased on master, so better delete/recreate branch instead of pulling.

[nlepage]

OK, I think this is ready. @frinyvonnick maybe review this together ?