-
Notifications
You must be signed in to change notification settings - Fork 4
Time extension #91
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
base: main
Are you sure you want to change the base?
Time extension #91
Conversation
embray
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.
Bravo--super nice work.
| ASDF_TIME_FORMAT_PLOT_DATE, | ||
| ASDF_TIME_FORMAT_YMDHMS, | ||
| ASDF_TIME_FORMAT_datetime64, | ||
| } asdf_time_base_format; |
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 like to add the _t suffix even with type-def'd enums. But I don't have a strict rule about this. Just mentioning it but I won't nitpick over it beyond that ;)
| struct asdf_time_info_t { | ||
| struct timespec ts; | ||
| struct tm tm; | ||
| }; |
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.
Ah, but then don't use the _t suffix for anything that isn't typedef'd; then it's hard to remember it needs struct :)
src/core/time.c
Outdated
| const int HOURS_PER_DAY = 24; | ||
| const int SECONDS_PER_DAY = 86400; | ||
| const int SECONDS_PER_HOUR = 3600; | ||
| const int SECONDS_PER_MINUTE = 60; |
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.
More stuff should be static around here.
| "t_jd", | ||
| "t_mjd", | ||
| "t_byear", | ||
| }; |
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 you want you can also write parametrized tests with μnit. Not so important here but you can see an example in test-ndarray.c for example (test_numeric_conversion_params).
tests/test-time.c
Outdated
|
|
||
| value = asdf_get_value(file, key); | ||
| if (asdf_value_as_time(value, &t) != ASDF_VALUE_OK) { | ||
| fprintf(stderr, "asdf_value_as_time failed: %s\n", key); |
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.
Oh yeah, you can use
munit_logf("asdf_value_as_time failed: %s\n", key);for this and other print calls in the tests. And below this you can return MUNIT_FAIL
* Use asdf_time_info_t structure member
# Conflicts: # src/core/history_entry.c
* Prefixed all time functions and variables with ASDF_TIME_ * Move a few reusable constants to time.h * show_tm() and show_timespec now accept a FILE stream to write to * Renamed show_asdf_time_info() to asdf_time_info_dump() * Conversion functions now have _convert_ in their names * Renamed struct asdf_time_info_t to asdf_time_info * Use asdf_value_as_mapping() in place of asdf_value_is_mapping() * Add tests: test_tm_to_julian() and test_tm_to_besselian() * Replace calls to printf with munit_logf
No description provided.