Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): add file property for notes #614

Merged
merged 2 commits into from
Nov 23, 2016

Conversation

dignifiedquire
Copy link
Contributor

Fixes #609

if (typeof val.file === 'string') {
var filename = path.join(process.cwd(), val.file);
try {
val.description = fs.readFileSync(filename).toString();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is currently sync because this pipeline is currently all sync. I can also move it to the async version if that's preferred but that would mean changing more in this file and the interface to sort.

@@ -36,6 +38,15 @@ module.exports = function sortDocs(comments, options) {
var fixed = options.toc.filter(function (val) {
return typeof val === 'object' && val.name;
}).map(function (val) {
if (typeof val.file === 'string') {
var filename = path.join(process.cwd(), val.file);
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this more, in a case like

documentation build --config=some/directory/documentation.yml index.js

The paths in documentation.yml should be relative to some/directory rather than to ./. In that case, we might want to add the config path when we go through configParser (docs for that on yargs), so that sort can use it as a relative path if it's present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it so that

  • if loading through the config, the file is resolved by that path.
  • sort.js checks for absolute paths and uses them if found, otherwise it resolves against process.cwd()

@tmcw
Copy link
Member

tmcw commented Nov 23, 2016

👍 Awesome, thanks! This is great work.

Out of curiosity: the naming convention that you're using for the commit message (like feat(config):) are you targeting commitizen or standard-changelog or something else? I've been meaning to switch to one of those and am noticing more folks using that style, but not sure which tool is the standard.

@tmcw tmcw merged commit d96aa47 into documentationjs:master Nov 23, 2016
@dignifiedquire
Copy link
Contributor Author

@tmcw yes long ingrained habit those commit messages :)

I've been using the conventional-changelog family of tooling for some time in all my projects and are pretty happy with it. If you want some details and infos on tooling around this I collected some info recently here: ipfs/aegir#30

@dignifiedquire dignifiedquire deleted the feat/notes-files branch November 23, 2016 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants