config file support in ecp, local & validate modes#25
config file support in ecp, local & validate modes#25dor-itzhaki wants to merge 1 commit intomasterfrom
Conversation
dor-itzhaki
force-pushed
the
config-file
branch
from
c7d60c9 to
837edfc
Compare
| .describe('fs', 'one or more folders containing the source files to extract docs from') | ||
| .alias('fx', 'excludes') | ||
| .describe('fx', 'one or more folders to exclude (including their children) from extracting docs') | ||
| .default('fp', '.+\\.js?$') |
There was a problem hiding this comment.
did you remove this default on purpose?
| .describe('fx', 'one or more folders to exclude (including their children) from extracting docs') | ||
| .default('fp', '.+\\.js?$') | ||
| .alias('fp', 'pattern') | ||
| .describe('fp', 'file pattern, defaults to ".+\\.js$"') |
There was a problem hiding this comment.
if you did remove it on purpose (see previous comment) then you also need to modify the text here
| const argv = commandConfig.parse(cliArgs) | ||
|
|
||
| const config = argv.config | ||
| ? Object.assign(require(argv.config), argv) |
There was a problem hiding this comment.
config file is assumed to be what? a path to a JS that exports an object?
Should be documented better in the command description, and probably also elsewhere
| let argv = optimist | ||
| .usage('Usage: $0 ecp -r [remote repo] [-b [remote branch]] -s [local sources] -p [file pattern] [-ed [enrichment docs directory]] [--plug [plugin]] [--dryrun]') | ||
| .demand('r') | ||
| const commandConfig = optimist |
There was a problem hiding this comment.
according to https://github.com/substack/node-optimist optimist is deprecated, it might be worth replacing it already (it won't be a lot of work). Acceptable to do separately IMO.
| : argv | ||
|
|
||
| if (!config.remote || !config.sources || !config.project) { | ||
| commandConfig.showHelp() |
There was a problem hiding this comment.
To my understanding, this will make the command description pop up, which is why it should be more descriptive
| projectSubdir: config.project, | ||
| jsDocSources: { | ||
| include: config.sources, | ||
| includePattern: config.pattern || '.+\\.js?$', |
There was a problem hiding this comment.
I now understand why you removed the default from the fp argument, but I still think it's pretty confusing in the command description. Leaving the comments in place as it made me confused, but I can see why you'd want to keep it as is - think about it.
| @@ -0,0 +1,251 @@ | |||
| const chai = require('chai') | |||
There was a problem hiding this comment.
we should open a tech-debt task to move docworks to jest. Could be a candidate first-task for a newcomer
| return tempFilePath | ||
| } | ||
|
|
||
| describe.only('config file unit tests', function () { |
There was a problem hiding this comment.
The only should probably be removed, I assume you used it for debugging.
There was a problem hiding this comment.
Another tech-debt task is to make sure eslint catches this..
dor-itzhaki
force-pushed
the
config-file
branch
from
837edfc to
0d650c6
Compare
2 participants
No description provided.