fix: simplify validate-addons logic & npm-exists #67
Conversation
|
Want to remove the mock implementation from so much here, but the entire parser is circular, we should try to use ´jest.mockfn` instead for some of this, so we can remove some manual mocks. |
evenstensberg
changed the title
lib/utils/validate-addons.js
Outdated
| @@ -1,30 +1,28 @@ | |||
| const exists = require('./npm-exists'); | |||
| const resolvePackages = require('./resolve-packages'); | |||
|
|
|||
| module.exports = (addons) => addons.map( pkg => checkExistence(pkg)); | |||
There was a problem hiding this comment.
- Let's always use
functiondeclarations when defining functions - Let's name this function
validateAddons - There is no need to create the closure in map / create a separate function
- Please add JSDoc to the exported function since this is the API
module.exports = function validateAddons(addons) {
return addons.map(checkExistence);
}
|
@okonet Changed. I like abstracting each check to an "pure" function in a sense that it looks less verbose. About the inline arrow functions, I agree, but I assumed the |
lib/utils/validate-addons.js
Outdated
| * @returns { <Function|Error> } resolvePackages - Returns an process to install the pkg | ||
| */ | ||
|
|
||
| module.exports = function validateAddons(addons) { |
There was a problem hiding this comment.
Do we need to export this function? It seems that it could be easily done in the code where you have to check.
Also, why not addons.map(checkExistence)?
lib/utils/validate-addons.js
Outdated
|
|
||
| module.exports = function validateAddons(addons) { | ||
| return addons.map( pkg => checkExistence(pkg)); | ||
| }; | ||
|
|
||
| /* | ||
| * @function checkExistence |
There was a problem hiding this comment.
I would export this function and do the map elsewhere. Should we rename it to npmPackageExist?
lib/utils/validate-addons.js
Outdated
| * on npm and throws an error if it is not from @checkExistence | ||
| * | ||
| * @param { Array } pkg - Array of packages to check existence of | ||
| * @returns { <Function|Error> } resolvePackages - Returns an process to install the pkg |
There was a problem hiding this comment.
It will return an Array, actually.
lib/utils/validate-addons.js
Outdated
| * Loops through an array and checks if a package is registered | ||
| * on npm and throws an error if it is not from @checkExistence | ||
| * | ||
| * @param { Array } pkg - Array of packages to check existence of |
evenstensberg
force-pushed
the
fix/tests
branch
from
0373aac
to
df0a0ad
Compare
Simplifies the usage of validate-addons and replaces the manual mock of nom-exists with the actual. fix: refactor function convention and add jsdcos fix: rename validateAddons to npmPackagesExists Renames the validateAddons packages to npmPackagesExists. Synced with master to get latest changes from origin. Also added lint ignore to coverage folder. fix: remove coverage folder from commit fix: rename test header
evenstensberg
force-pushed
the
fix/tests
branch
from
2cd4316
to
679a8d1
Compare
|
I wouldn't commit coverage to the repo. The that it should work is to generate it during the check. Let's revert the .gitignore and merge. We need to work on the CI setup a bit more I think. |
evenstensberg
force-pushed
the
fix/tests
branch
from
770b236
to
64d036e
Compare
|
@okonet Done. I'm leaving the merging to you :) Dunno why the coverage is included in the PR, but I removed it in |
|
I still can see the coverage file in the changes tab :/ |
|
yeah, me2 :( Let me try deleting the folder. |
fix: remove coverage folder
evenstensberg
force-pushed
the
fix/tests
branch
from
e4b7e58
to
f72ac6c
Compare
|
there we go 👍 |
|
LGTM please squash and merge! |
* hotfix: simplify validate-addons logic & npm-exists Simplifies the usage of validate-addons and replaces the manual mock of nom-exists with the actual. fix: refactor function convention and add jsdcos fix: rename validateAddons to npmPackagesExists Renames the validateAddons packages to npmPackagesExists. Synced with master to get latest changes from origin. Also added lint ignore to coverage folder. fix: remove coverage folder from commit fix: rename test header * fix: remove coverage from build fix: remove coverage folder
Simplifies the usage of validate-addons and replaces the manual mock of
nom-exists with the actual.