-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation
Test coverage is still removed, as it needs to be rewritten to use a different (perhaps, a more simple) solution than the current |
@@ -193,7 +196,7 @@ class World { | |||
|
|||
// Debugging helper | |||
inspect(data) { | |||
if (typeof data === 'object') { | |||
if (data !== null && typeof data === 'object') { |
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.
Added explicit comparison to null
to prevent false positives (typeof null
is "object"
).
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.
I'm confused about the lodash, otherwise looks good.
@@ -100,7 +99,9 @@ class SchemaV4Generator { | |||
const schemaType = this.getSchemaTypeFor(baseObject); | |||
|
|||
const hasKeys = | |||
type.object(baseObject) && Object.keys(baseObject).length > 0; | |||
baseObject !== null && |
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.
should this be !==
or !=
? (thinking of undefined
)
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.
well typeof undefined
is 'undefined'
so I guess it's sorted by the other line
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.
First checking for null
to prevent false positive on the next line. Yes, then the next line is satisfied only on object
. undefined
value would reject then.
"json-parse-helpfulerror": "1.0.3", | ||
"json-pointer": "0.6.0", | ||
"media-typer": "0.3.0", | ||
"tv4": "1.3.0" | ||
}, | ||
"devDependencies": { | ||
"chai": "4.2.0", | ||
"coffee-coverage": "2.0.1", |
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.
Closes #114
test/cucumber/support/world.js
Outdated
@@ -5,6 +5,9 @@ | |||
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md | |||
*/ | |||
const gavel = require('../../../lib/gavel'); | |||
// DO NOT REMOVE! | |||
// lodash is not used, but somehow accessed from Cucumber feature testing. | |||
// May be a case of nasty shadow module references. | |||
const _ = 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.
how can the line work if we removed lodash from deps?
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.
Okay, I'm testing now, and it seems to pass. Sorry for the confusing comment. Will remove.
80f9993
to
f8064ef
Compare
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changelog
is-type
)lodash
)