aglio icon indicating copy to clipboard operation
aglio copied to clipboard

Breaking: support for Hercule transclusion w/ backwards compatibility (WIP)

Open jamesramsay opened this issue 9 years ago • 7 comments

I noticed some discussion in the API Blueprint Slack chat room about supporting Hercule syntax in Aglio, and can see how it's unfortunate for new comers to Apiary and API Blueprint to make the choice between Aglio's handy rendering capabilities and Hercule's syntax which play's nicer with Markdown.

Rather than leaving you to work out how to replicate the Hercule syntax or a subset thereof, or add Hercule as a dependency, I thought I'd take look.

I've made a few small changes to Hercule to make using it in this application simpler:

  • ability to retrieve a pathList (required by collectPathsSync): https://github.com/jamesramsay/hercule/pull/198
  • backwards compatibility with existing Aglio syntax by allowing a custom regular expression for tokenising to be provided: https://github.com/jamesramsay/hercule/pull/201

These changes were quite small, and made it quite easy to drop Hercule into Aglio. I've published these changes to NPM as 3.0.0-alpha.1 (w/ next distribution tag) and a breaking change to the async API to use typical error-first, single-parameter callback style.

However, because Hercule is entirely built on stream and the API is dominantly asynchronous, I've made collectPaths asynchronous in the PR. Adding the pathList to Hercule's sync API is a little bit more complicated.

I'd be interested on your thoughts on this contribution. Thanks for you consideration!


Items that should likely be addressed before merging:

  • [ ] Add test documented circular reference error handling
  • [ ] Update documentation about support for HTTP transclusion and new syntax

jamesramsay avatar Mar 13 '16 06:03 jamesramsay

Regarding the breaking sync API change proposal, I noticed looking at the Travis config, Aglio supports node 0.10. Hercule's method for offering a sync API relies on childProcess.spawnSync which is only available in node 0.12 and above. :no_good: :disappointed:

jamesramsay avatar Mar 13 '16 06:03 jamesramsay

:+1: For me I am all up for using Hercule transclusions in Aglio.

zdne avatar Mar 17 '16 10:03 zdne

Thanks @zdne :tada: Any thoughts from @danielgtaylor ?

I've covered everything these changes touch, but the uncovered LOCs remains equal, while covered LOCs decreases, thus the small decrease in coverage.

jamesramsay avatar Mar 18 '16 02:03 jamesramsay

Coverage Status

Coverage decreased (-0.1%) to 92.336% when pulling 06808be4de29f6db523128e96f8c7fb35c5412af on jamesramsay:hercule into ccd8c2a728c0003cab508cb65b497df2bdb730d9 on danielgtaylor:master.

coveralls avatar Mar 23 '16 01:03 coveralls

@jamesramsay I think this is great. I'd like to hold off on merging just for a bit, get an updated stable release out and then move forward with breaking changes to the API. :+1:

danielgtaylor avatar May 20 '16 18:05 danielgtaylor

this looks very interesting. Cheeky bump!

philsturgeon avatar Jul 21 '16 10:07 philsturgeon

Coverage Status

Coverage decreased (-0.1%) to 92.336% when pulling f7d7395353bd961ef11b277ebb67134ee66b60a9 on jamesramsay:hercule into df554dc380c0ee6a2c83f8c62c4739491c5dd3c2 on danielgtaylor:master.

coveralls avatar Nov 29 '16 23:11 coveralls