Adding RFC6570 style uri templates to store - issue 275 #328
Conversation
|
Looks cool. @nickstenning how do you feel about something like this? Worth doing? Breaks compatibility. But interesting to me because an RFC is nice to have as a reference when pursuing bootstrapping the store automatically, given that annotator-store already publishes an API discovery document. |
|
I think this is breakage worth having. P.S. This is an example of why the browserify work was worth doing! |
|
Ah, although there is a slight subtlety here. The existing implementation has a slightly kooky behaviour for the read URL, namely The point here is that if you don't specify |
|
And just to be clear, I'm not defending the current behaviour -- merely raising an issue that might want to be considered before we merge this. |
|
So, I'm totally keen on making this work, and making the new standard URL definitions RFC6570-compliant. In order to ship this in 2.0, however, I'd like us to provide backwards compatibility for the old style, and issue a deprecation warning. Perhaps the best way of doing this would be to try parsing the provided URLs with the RFC-compliant parser, and then inspect the templates. If they don't have the right fields (i.e. one for |
|
You may also wish to make sure you're aware of what will (hopefully) soon be merged in #361 before updating this PR. |
| # If there's a bare ':id' in the URL, then substitute directly: | ||
| url = url.replace(/:id/, if id? then id else '') | ||
| t_url = template.parse(url) | ||
| t_url.expand({id: id}) |
There was a problem hiding this comment.
Tiny style comment, the project uses camel case for variable and method names (t_url -> tUrl).
|
Also think this is a great idea. Just wanted to question the use of adding a 1kb (gzipped) dependency to replace two regular expressions. I think it's totally warranted if the library is going to be used in multiple places across the project but if it's just here then surely it would make more sense to just update the RegExp? It can still be RFC compliant but will keep the number of external dependencies down and the code easy to follow. |
|
@nickstenning you closed the issue, but this PR is open. FWIW I'm still in favor of heading this direction, I think... though I'm curious what @BigBlueHat thinks since I know he's been playing with RAML and likes to think about schemas and descriptors and whatnot. |
|
@tilgovi no idea how I missed your mention in September... Regardless, huge 👍 for this change (thanks for merging it @nickstenning), and another 👍 for pursuing more document / definition centric development, documentation, and deliverables. 😸 /me goes back to hunting down his RAML work for Annotator... |
|
Found it! hypothesis/h#1516 |
Changes the uri templates in store to match RFC6570.
Fixes #275.