lirantal
commented
4 years ago This is wonderful @mtlewis, thanks for working on this ❤️ Lovely picture too!
I'll review it shortly
Closed mtlewis closed 4 years ago
lirantal
commented
4 years ago This is wonderful @mtlewis, thanks for working on this ❤️ Lovely picture too!
I'll review it shortly
lirantal
commented
4 years ago @mtlewis I'm happy to land this. Are you able to add the relevant tests to match up with code coverage requirement? that's why the build is failing now.
codecov-io
commented
4 years ago Merging #75 into master will increase coverage by
1.79%. The diff coverage is100%.
@@ Coverage Diff @@
## master #75 +/- ##
==========================================
+ Coverage 95.08% 96.87% +1.79%
==========================================
Files 11 11
Lines 183 192 +9
Branches 29 31 +2
==========================================
+ Hits 174 186 +12
+ Misses 8 5 -3
Partials 1 1| Impacted Files | Coverage Δ | |
|---|---|---|
| packages/lockfile-lint/src/config.js | 100% <100%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 8e3cdbe...76df197. Read the comment docs.
mtlewis
commented
4 years ago Hey @lirantal, thanks for your review! I've added some tests. I also did a bit of refactoring of the PR, since the way it was before made it harder to retain the argument validation that's configured in yargs. Instead, we now load the cosmiconfig first, then pass it into yargs as the base config. This allows us to keep the configuration rules in yargs. Let me know what you think!
mtlewis
commented
4 years ago By the way - I'm not sure coverage is currently working in cli.js. I believe I've covered all the new code in cli.js in tests, but both before and after this change, the detected coverage in cli.js is 0. I suspect this could be something to do with the fact that the tests for cli.js are currently executing in a separate process.
I guess the reason CI is failing in this PR is that cli.js now has more lines of code, which means its 0% coverage contributes more to the overall number.
lirantal
commented
4 years ago Yep, I see what you mean there about the cli.js not being covered in unit tests (and indeed in a different process they aren't being counted for).
What if you move this code https://github.com/lirantal/lockfile-lint/blob/0a90e4291cbf1d41a9246d5a38f7567f11781cc5/packages/lockfile-lint/src/cli.js#L5-L20 to its own config.js module, add unit test coverage to it, and then require it in cli.js?
Tip: to better understand what lines of code are missing coverage, you can run yarn run coverage:view
mtlewis
commented
4 years ago Hey @lirantal thanks for the feedback and suggestion - how about this: 76df197? Since the whole of cli.js was previously uncovered, I figured it might be good to refactor the whole thing so that it's testable without launching a child process, and then separate out the testing of the option parsing/validation from the cli tests in cli.js.
lirantal
commented
4 years ago Ok, taking a look ;-)
lirantal
commented
4 years ago this looks overall good, thanks @mtlewis 🙏
Description
This PR adds support for loading lockfile-lint configuration from a file, rather than supplying it on the command line. It uses cosmiconfig, which gives us a bunch of sensible configuration file support for free. Command-line options always override options specified in config files.
Types of changes
Related Issue
Welp - I didn't notice that only PRs with open issues would be accepted before I worked on this, so I created one (#74). Sorry for not quite following the guidelines!
Motivation and Context
See #74.
How Has This Been Tested?
So far, I've only tested this manually. I'll be happy to circle back and add tests if there's a positive reaction to #74.
Checklist: