-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix: Added date time parsing for conditional formatting #1120
fix: Added date time parsing for conditional formatting #1120
Conversation
// Proper date validation will be addressed by Issue #1108 | ||
return value != null && value !== ''; | ||
default: { | ||
const [dt, tz] = (value ?? '').split(' '); |
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.
One suggestion - use camel case and full words for variable names. We try to follow the Airbnb style guide – https://github.com/airbnb/javascript#naming-conventions
with a few exceptions. e
as the catch
block argument is fine, single-character names in loops and maps are also fine, as long as the loop body is fairly short.
In this case, these are local variables and the scope is short so it's probably fine, but I thought I'd mentioned this anyway.
Codecov Report
@@ Coverage Diff @@
## main #1120 +/- ##
==========================================
+ Coverage 43.37% 43.39% +0.01%
==========================================
Files 434 434
Lines 32617 32626 +9
Branches 8218 8218
==========================================
+ Hits 14148 14157 +9
Misses 18420 18420
Partials 49 49
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
// Proper date validation will be addressed by Issue #1108 | ||
return value != null && value !== ''; | ||
default: { | ||
const [dateTimeString, tzCode] = (value ?? '').split(' '); |
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 2023-02-23T11:46:31.000000000 NY FOO
pass validation but still fail server size?
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.
Good catch.
Also, a dateTime string can have a space in place of the t-separator: 2023-02-23 11:46:31.000000000 NY
.
Regex for the separator in DateUtils/parseDateTimeString
looks like this: [tT\s]
.
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.
Ignore my comment, looks like the engine expects a T
in format conditions.
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 would be nice if we could accepted strings anywhere in the UI that takes a datetime with or without the T separator, then normalize it before sending.
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.
Agree, also accept datetime with or without the time zone. Should default to the time zone in the user settings. That would be another ticket though.
a332ccf
to
cc2c1c2
Compare
@vbabich It was suggested by @niloc132 that instead of using I'm hesitant to do that for this issue given the current solution is already implemented, but I can create a new issue if we think this is a better solution. |
@bmingles Agreed, open another issue for this. |
Added datetime string parsing / validation.
fixes #1108