calmm-js / partial.lenses

Partial lenses is a comprehensive, high-performance optics library for JavaScript
MIT License
915 stars 36 forks source link

Feature/json pointer #121

Closed kurtmilam closed 7 years ago

kurtmilam commented 7 years ago

This is a WIP. See the JSON Pointer RFC (6901) here.

Open:

Question: There are two formats for these pointers. Only one is supported in the original PR. Should support be added for the other? (I assume it probably should) This has been implemented.

polytypic commented 7 years ago

Hmm... Is the other format just L.pointer(decodeURIComponent(fragment.substring(1)))?

kurtmilam commented 7 years ago

I think so, with a leading '#'. Super easy to implement if that's the case, and I'll be happy to add it, probably tomorrow.

polytypic commented 7 years ago

Maybe L.pointerURI then?

polytypic commented 7 years ago

Or actually just:

if (s[0] === '#') s = decodeURIComponent(s.substring(1))

Because, IIUC, a JSON Pointer cannot otherwise start with #.

kurtmilam commented 7 years ago

Yeah, that was the question I hadn't decided on yet - one function for both or one for each? I agree that a separate one for the URI fragment format makes sense. I'll make it so.

I'm also fine with doing a single function that handles both formats. :D

polytypic commented 7 years ago

Actually it seems that a JSON Pointer cannot start with a #, so it is probably better to have just L.pointer do the detection internally.

kurtmilam commented 7 years ago

Before I touch the docs - I need to make my updates to README.md? Is the version number added automatically? Anything else I need to know?

edit: I just noticed the instructions for working with the documentation while looking at README.md in my editor. It's not clear to me whether I need to manually add the version number in which the new method will be included, or if that's done automagically.

Also, anything I need to know before I add tests?

edit: I went ahead and added tests: See my latest commit. Let me know if you see room for improvement.

polytypic commented 7 years ago

If you mean the version numbers indicating when combinators were added in the contents and the headers then they are added by hand for new combinators (I used some ad hoc scripts to get the version numbers initially).

You can use version 11.21.0.

polytypic commented 7 years ago

It is best not to include generated files in dist and docs directories in PRs.

kurtmilam commented 7 years ago

I'll address these and and recommit. Does it make sense to add the dist and docs folders to .gitignore?

I suspected that those folders weren't supposed to be included in PR commits, but I thought that would probably be reflected in .gitignore if it were the case.

polytypic commented 7 years ago

IIRC, the dist files were added to the repository to support Bower. (Strictly speaking there is no need to have the cjs and es versions in the repository.) The need to have Bower support is diminishing (there is at least one project I know that is currently using Bower (and Partial Lenses), but is transitioning to Yarn and WebPack), so once the Bower support is dropped, the whole dist directory can be removed from the repository.

kurtmilam commented 7 years ago

Understood. I hesitated to include those files in the commit. Fortunately, it's an easy mistake to fix.

kurtmilam commented 7 years ago

docs: I'm not sure what the signature (anchor title) of pointer is. I've looked through the signatures of a bunch of other functions, but I'm still uncertain. Should this go in the 'Auxiliary' section after seemsArrayLike or somewhere else?

polytypic commented 7 years ago

Hmm... I'd put pointer under a new section titled "Interop".

The signature could be like this:

[`L.pointer(jsonPointer) ~> lens`](#L-pointer "L.pointer: JSONPointer s a -> PLens s a") 
polytypic commented 7 years ago

This is looking good. 👍

I'll shortly add a couple of comments, but I can also make the suggested edits after merging, if you feel that this is otherwise ready.

polytypic commented 7 years ago

This looks good to me. I'll merge and make some minor adjustments. A big thanks for the contribution!