danielgtaylor / aglio

An API Blueprint renderer with theme support that outputs static HTML
4.75k stars 481 forks source link

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

Open jamesramsay opened 8 years ago

jamesramsay commented 8 years ago

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:

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:

jamesramsay commented 8 years ago

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:

zdne commented 8 years ago

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

jamesramsay commented 8 years ago

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.

coveralls commented 8 years ago

Coverage Status

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

danielgtaylor commented 8 years ago

@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:

philsturgeon commented 8 years ago

this looks very interesting. Cheeky bump!

coveralls commented 7 years ago

Coverage Status

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