Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

Add file ref support#7

Merged
Deadarius merged 11 commits intomasterfrom
add-file-ref-support
May 7, 2019
Merged

Add file ref support#7
Deadarius merged 11 commits intomasterfrom
add-file-ref-support

Conversation

@Deadarius
Copy link
Copy Markdown

@Deadarius Deadarius commented May 7, 2019

  • Refactor: replace supplied utils with lodash
  • Type improvements
  • Use json-schema-ref-parser to combine bundle schemas while preserving $refs
  • Now schema can be supplied as a path to file

Lint errors to be fixed

@Deadarius Deadarius force-pushed the add-file-ref-support branch from e1095f2 to 9544c11 Compare May 7, 2019 04:59
Copy link
Copy Markdown

@lwcooper lwcooper left a 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');
Copy link
Copy Markdown

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 👍

Copy link
Copy Markdown
Author

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

* @param options
*/
constructor (serverless, options) {
constructor (serverless: IFullServerless, options) {
Copy link
Copy Markdown

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?

Copy link
Copy Markdown
Author

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.

@Deadarius
Copy link
Copy Markdown
Author

RE: test covereage - #13

@Deadarius Deadarius merged commit 2f671fb into master May 7, 2019
@Deadarius Deadarius deleted the add-file-ref-support branch May 7, 2019 23:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants