Add Date Time Range type #2
Conversation
|
Can one of the admins verify this patch? |
|
add to whitelist |
src/types/Date Time Range.coffee
Outdated
| nativeProperties: | ||
| has: | ||
| Begin: (from) -> ['RangeBegin', from] | ||
| End: (from) -> ['RangeEng', from] |
There was a problem hiding this comment.
I'm guessing RangeEng should be RangeEnd?
src/types/Date Time Range.coffee
Outdated
| fetchProcessing: (data, callback) -> | ||
| if data? | ||
| res = | ||
| Start: data.split(',')[0].slice(1) |
There was a problem hiding this comment.
It would be better to do this .split only once, maybe something like:
[ start, end ] = data.slice(1, -1).split(',')
|
Moved dataTypeGen from abstract-sql-compiler based on this comment: balena-io-modules/lf-to-abstract-sql#3 (comment) |
| if !err | ||
| defaultValue = value | ||
| else | ||
| defaultValue = null |
There was a problem hiding this comment.
If the defaultValue is invalid we should actually throw an error rather than just silently ignoring it - if we've tried to use a default value it's probably required for things to work properly.
| @@ -18,4 +18,14 @@ | |||
| callback(err) | |||
| return | |||
| callback(null, value.toLocaleTimeString()) | |||
|
|
|||
| dataTypeGen: (engine, dataType, necessity, index = '', defaultValue) -> | |||
| if defaultValue | |||
There was a problem hiding this comment.
This would remove the option of having a default value of 0, false, '' etc, it should be if defaultValue? to check for nullish values directly (which is ok since a default value of null is equivalent to no default)
| @@ -18,4 +18,14 @@ | |||
| callback(err) | |||
| return | |||
| callback(null, value.toLocaleTimeString()) | |||
|
|
|||
| dataTypeGen: (engine, dataType, necessity, index = '', defaultValue) -> | |||
There was a problem hiding this comment.
From what I can see all of the default value checking in $type.dataTypeGen functions are the same, so it can be pushed directly into TypeUtils.dataTypeGen and have this bit just be dataTypeGen: TypeUtils.dataTypeGen
There was a problem hiding this comment.
We could have a default value equal to CURRENT_TIMESTAMP (e.g. in Date Time type) which will not get caught by the validate function. So these cases need a special treatment I think.
There was a problem hiding this comment.
Also, if I move everything to TypeUtils.dataTypeGen I won't be able to reference lets say the per type validate from there. (I assume, by looking how the types.js is being created)
| if defaultValue | ||
| @validate defaultValue, true, (err, value) -> | ||
| if !err | ||
| defaultValue = value |
There was a problem hiding this comment.
This only works because our "async" callback is actually synchronous, if you try to do a default value in the Hashed datatype then it will break because that is actually truly async (and so this will happen after we've already returned the datatype, making the default value be the unprocessed one). Therefore we need to turn this into an async function that accepts a callback - if you're feeling up for it it would also be great to turn it into a dual interface api that supports both promises and callbacks by doing something like
fn = (..., callback) ->
PromiseStuff...
.asCallback(callback)
There was a problem hiding this comment.
What exactly should be turned into an async function? The validate function you mean?
| callback('is not a date time range object: ' + value) | ||
| else | ||
| # Check with hasOwnProperty since null values are allowed | ||
| if value.hasOwnProperty('Start') and value.hasOwnProperty('End') and value.hasOwnProperty('Bounds') |
There was a problem hiding this comment.
This forces the properties into a specific case where otherwise we would allow any case (with the .toLowerCase() below). I think it does make sense to be case insensitive, so maybe after the for loop we can do an if !start? or !end? or !bounds? to check that all the properties were set
| when 'end' | ||
| end = componentValue | ||
| when 'bounds' | ||
| bounds = componentValue |
There was a problem hiding this comment.
It would be good to validate bounds here, because we access it using index properties below but if it's actually a number for instance then that will fail (and in general only certain values make sense)
| when 'start' | ||
| start = componentValue | ||
| when 'end' | ||
| end = componentValue |
There was a problem hiding this comment.
Can we do some validation of this?
| for own component, componentValue of value | ||
| switch component.toLowerCase() | ||
| when 'start' | ||
| start = componentValue |
There was a problem hiding this comment.
Can we do some validation of this?
No description provided.