Closed JamesLMilner closed 6 years ago
Merging #243 into master will increase coverage by
0.1%
. The diff coverage is95.31%
.
@@ Coverage Diff @@
## master #243 +/- ##
=========================================
+ Coverage 94.16% 94.26% +0.1%
=========================================
Files 20 21 +1
Lines 600 663 +63
Branches 95 104 +9
=========================================
+ Hits 565 625 +60
- Misses 17 18 +1
- Partials 18 20 +2
Impacted Files | Coverage Δ | |
---|---|---|
src/configurationHelper.ts | 96.77% <100%> (+0.1%) |
:arrow_up: |
src/command.ts | 94.44% <100%> (ø) |
:arrow_up: |
src/commands/validate.ts | 95.16% <95.16%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 14f3c88...1b81702. Read the comment docs.
Windows error is:
at describe (C:\Users\James\Documents\Code\intern-test\james\cli\dist\dev\tests\unit\commands\validate.js:83:31)
at registerSuite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:73:9)
at _suite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:85:13)
at suite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:58:24)
at describe (C:\Users\James\Documents\Code\intern-test\james\cli\dist\dev\tests\unit\commands\validate.js:81:9)
at registerSuite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:73:9)
at _suite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:85:13)
at suite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:58:24)
at describe (C:\Users\James\Documents\Code\intern-test\james\cli\dist\dev\tests\unit\commands\validate.js:76:5)
at registerSuite (C:\Users\James\Documents\Code\intern-test\james\cli\node_modules\intern\lib\interfaces\tdd.js:73:9)
message: 'expected \'tests\\\\support\\\\validate\\\\rc-schema.json\' to equal \'tests/support/validate/rc-schema.json\'',
showDiff: true,
actual: 'tests\\support\\validate\\rc-schema.json',
expected: 'tests/support/validate/rc-schema.json',
reported: true }
@JamesMilnerUK I’ve been thinking about the command validation and general API, which are different to how you’ve currently implemented it....
I’ve been thinking that we should provide a json schema validation function via the helper and the command API for validate should be a function instead of a boolean.
In the validate
function commands would/could use the provided json schema validation function from the CLI. This would make the validation flexible as a command can run completely bespoke validation or extra validation on top of the basic schema validation. It would also mean that you should have to worry about paths for the command.
General note, do you think that we need to report successful validation for each command? Especially when running the command (as the user doesn't actually need to know that it is validated before running)... for the validate built in I agree we should return a general, "All you commands are configured correctly" or what have you.
@agubler as it stands each command is responsible for deciding if silentSuccess
should be set. If that's set to true
then nothing will log on success regardless of how the validate command is run. Because we invert the control to the command to setup validate, we would have to say it's just best practice to set that to false
for installable commands, if that makes sense.
Feature
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
Resolves #241
This should allow commands to be validated against a validation schema that is provided in the command source (at the moment it's named
rc-schema.json
but we could go with something else). If no config is provided validation will skip for the command.