-
-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Jellyfish (UPLOAD-1, UPLOAD-1399) #1599
base: master
Are you sure you want to change the base?
Conversation
…pload through platform
…w only upload through platform" This reverts commit 39085e0.
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.
@gniezen see initial feedback / questions
@@ -130,9 +130,12 @@ export class App extends Component { | |||
? JSON.parse(localStore.getItem('selectedEnv')) | |||
: null; | |||
|
|||
this.props.async.fetchInfo(() => { | |||
this.props.async.fetchInfo((err, info) => { |
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.
it appears we aren't doing anything with the err
?
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.
If there is an error we already dispatch an error message in fetchInfo
itself, so I'll just log it to the console.
lib/commonFunctions.js
Outdated
} | ||
}; | ||
|
||
exports.strip = function(record) { |
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.
stripUnwantedFields
or deleteUnwantedFields
... or just something a bit more descriptive?
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.
test?
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 a test.
lib/core/device.js
Outdated
/* eslint-disable global-require, import/no-unresolved, import/extensions */ | ||
device.deviceDrivers.Tandem = require('../drivers/tandem/tandemDriver'); | ||
device.deviceDrivers.TandemJellyfish = require('../drivers/tandem/jellyfish/tandemDriver'); | ||
} catch (e) { |
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.
error detail is of no use? also should it be err
for the sake of consistency?
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.
Yeah, not really of any use, as it will just say the file can't be found if you don't have access to the private submodule. I'll rename it to err
for consistency.
lib/core/device.js
Outdated
|
||
const dm = device.createDriverManager(driverId, options); | ||
if (dm == null) { | ||
cb(new Error('Driver not available.')); |
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.
would the context of the driverId
we are trying to create help here in the error?
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.
Sure, I can add it to the error message.
@@ -177,3 +177,128 @@ exports.addDurationToDeviceTime = function (event, duration) { | |||
}; | |||
|
|||
exports.fixFloatingPoint = (n) => Math.floor(n * 100 + 0.5) / 100; | |||
|
|||
Number.prototype.toFixedNumber = function(significant){ |
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 know it's not exported but is there a test?
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 a test.
return +( Math.round(this*pow) / pow ); | ||
}; | ||
|
||
exports.updateDuration = function(event, lastEvent) { |
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.
is there a corresponding test?
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 a test.
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 think just one minor typo but LGTM
Addresses UPLOAD-1. Supersedes #719 .