-
Notifications
You must be signed in to change notification settings - Fork 25
Conversation
e1095f2
to
9544c11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lack of tests is a bit worrying in this whole module really! But not gonna block this PR being merged
@@ -1,10 +1,11 @@ | |||
import { JSONSchema7 } from 'json-schema'; | |||
import _ = require('lodash'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this syntax looks weird. how does it work? I've never seen it before
also, we normally use ramda 🙈 But I can see how much it makes sense to just replace the existing utils with lodash here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an open source project, I would prefer to keep it easy for people to contribute - lodash
is more aligned with this idea than ramda
|
||
/** | ||
* Constructor | ||
* @param serverless | ||
* @param options | ||
*/ | ||
constructor (serverless, options) { | ||
constructor (serverless: IFullServerless, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come options
doesn't need a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project is not configured(yet?) to use strict
typescript transpilation.
RE: test covereage - #13 |
json-schema-ref-parser
to combine bundle schemas while preserving $refsLint errors to be fixed