Skip to content

Conversation

@jhunkeler
Copy link
Contributor

No description provided.

Copy link
Collaborator

@embray embray left a 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;
Copy link
Collaborator

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;
};
Copy link
Collaborator

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;
Copy link
Collaborator

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",
};
Copy link
Collaborator

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).


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);
Copy link
Collaborator

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants