Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Extended JS interface -> Markdown generator #4275

Merged
merged 3 commits into from
Jan 24, 2017
Merged

Extended JS interface -> Markdown generator #4275

merged 3 commits into from
Jan 24, 2017

Conversation

maciejhirsz
Copy link
Contributor

Related to #2646 and #1905.

This PR contains mostly just the upgrades made to the markdown generator over time as I was adding missing methods, descriptions and examples to the js interfaces.

@maciejhirsz maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Jan 23, 2017
@@ -29,6 +29,8 @@
"build:app": "webpack --config webpack/app",
"build:lib": "webpack --config webpack/libraries",
"build:dll": "webpack --config webpack/vendor",
"build:markdown": "babel-node ./src/jsonrpc/generator/build-markdown.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not for this PR, but we should probably move the build-*.js out into (maybe?) js/scripts - it ended up being in there for no reason apart from the fact that we didn't want to lose code while combining the repos. (And as you've noticed was quite outdated and not being run)

Since it is building and not really source, it is a bit misleading atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of on the same accord - I reckon it would make sense to move the generated md files out from js/src/jsonrpc/docs to js/docs?


// Grabs JSON compatible
function getExample (obj) {
if (Array.isArray(obj)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to do const { isArray } = Array above and then directly using Array.isArray here? (Just feels slightly inconsistent in use)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just oversight.

}

// Logging helpers
function info (log) { console.log(chalk.blue(`INFO:\t${log}`)); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment, which would be more applicable to code not for the generator (i.e. these scripts I probably expect to always be a bit more messy - just the nature of the beast.) Would generally prefer even the simple functions to have the format of

function info (log) { 
   console.log(chalk.blue(`INFO:\t${log}`));
}

In addition blank line between. (non-critical here, nor a showstopper)

function warn (log) { console.warn(chalk.yellow(`WARN:\t${log}`)); }
function error (log) { console.error(chalk.red(`ERROR:\t${log}`)); }

const type2print = new WeakMap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably make sense to move this definition (& init) to above the log functions, just so the variables are more-or-less in the same place.

@jacogr
Copy link
Contributor

jacogr commented Jan 24, 2017

Couple of small comments, non-critical, but it looks like it does what it says on the tin.

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 24, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jan 24, 2017
@jacogr jacogr merged commit a30a108 into master Jan 24, 2017
@jacogr jacogr deleted the mh-rpcdocs branch January 24, 2017 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants