-
Notifications
You must be signed in to change notification settings - Fork 78
Add Support for DATE_TIMEs in STATA #325
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
Conversation
evanmiller
left a comment
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.
Thanks for the contribution. I left some suggestions.
src/bin/read_csv/mod_dta.c
Outdated
| fprintf(stderr, "%s:%d not a valid date-time: %s (expected format: yyyy-mm-dd hh:MM:SS with optional milliseconds. Datetime string is truncated at 23 characters to ignore microseconds and timezone information.)\n", __FILE__, __LINE__, date_time); | ||
| exit(EXIT_FAILURE); | ||
| } | ||
| int missing_ranges_count = readstat_variable_get_missing_ranges_count(var); |
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.
Why was this logic removed?
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 had initially copied the value_double_dta to value_double_date_time_dta to ensure code was being reached, before implementing value_double_date_time_dta as its own function. I wasn't sure how this code would handle blank date fields in the CSV.
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.
OK, I'm guessing missing ranges are seldom used with dates, so let's leave it out
|
@evanmiller Thanks for the feedback and suggestions, I'll be the first to admit my C skills are very, very rusty. If there's value in re-inserting the |
|
The macOS build is unhappy for some reason; not sure if related to your change or not |
@evanmiller Any tips on how I can best figure these out? I've been compiling on Rocky 9 but don't have an easy way to test on MacOS. Thanks again for your guidance and expertise. |
|
Looks like a pre-existing error, so I'm not going to sweat it. Thanks for the contribution. |
When converting a CSV to STATA, I received the following error:
src/bin/read_csv/mod_dta.c:404 unsupported variable type 3This PR adds support for DATE_TIMEs to the STATA writer. Here's the test CSV that was used:
Here's the JSON mapping file:
{ "type": "STATA", "variables": [ { "type": "NUMERIC", "name": "orgpermid", "label": "OrgPermID (orgpermid)", "format": "UNSPECIFIED" }, { "type": "NUMERIC", "name": "valuecalcdt", "label": "ValueCalcDt (valuecalcdt)", "format": "DATE_TIME" }, { "type": "NUMERIC", "name": "justdate", "label": "JustDate (justdate)", "format": "DATE" } ] }...and the output in STATA from the written file:
...and to validate millisecond display:
In reviewing the code, I also noticed the patterns for the three DATE_TIME patterns did not match the
Date-Time Unitfrom the source spec here: https://libguides.library.kent.edu/SPSS/DatesTimeI've also changed those patterns to match the spec.