-
Notifications
You must be signed in to change notification settings - Fork 42
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
[Datagen] time, date, timestamp range support with strings. #2357
Conversation
Fixes #2318. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Ok(Some((a, b))) | ||
} | ||
Some((Value::String(a), Value::String(b))) => { | ||
let unix_date: NaiveDate = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap(); |
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 it's malformed will you get a decent message?
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.
line 74 and 76 will do the error handlign for malformed user strings
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.
what is the user experience?
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.
they'll get an error from the adapter -- currently they'll have to inspect the manager log but once we land error handling in manager this will be there too
@@ -650,7 +770,8 @@ impl RecordGenerator { | |||
if let Value::String(str) = obj { | |||
str.clear(); | |||
|
|||
let (min, max) = settings.range.unwrap_or((0, MAX_DATE_VALUE)); | |||
let range = parse_range_for_date(field.name.as_str(), &settings.range)?; | |||
let (min, max) = range.unwrap_or((0, MAX_DATE_VALUE)); |
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 0 1970?
What if the range starts earlier?
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.
this is the default range if the user doesn't provide one
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.
but will it work with negative values?
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 will add a test
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Fixes #2318.
Is this a user-visible change (yes/no): ___