time handling #179
time handling #179
Conversation
There was a problem hiding this comment.
Generally I am happy with the approach of creation separate classes Duration/DateTime for handling time in the nice matter.
We need a very good test suite for just these 2 classes to be sure it works as we expect (so we need to unittest directly these classes)
What I would suggest is to use internally some C++/C time/datetime library and have Duration/DateTime just as wrapper around this library (if we can find such library). The convertion of these times are a bit tricky, especially with timezones (if we want to support that). look on https://github.com/HowardHinnant/date or similar to see if it is any help
@wonder-sk mentioned there is some article from google devs why to not try to create your own time library.
|
@wonder-sk I will be happy if you have linked toward this article |
mdal/frmts/mdal_cf.cpp
Outdated
| @@ -161,7 +161,78 @@ static void populate_vals( bool is_vector, double *vals, size_t i, | |||
| } | |||
| } | |||
|
|
|||
| void MDAL::DriverCF::addDatasetGroups( MDAL::Mesh *mesh, const std::vector<double> ×, const MDAL::cfdataset_info_map &dsinfo_map ) | |||
| static MDAL::DateTime parseReferenceTime( std::string timeInformation, std::string calendar ) | |||
|
@vcloarec unfortunately I can't find the original article... but this one also give quite a good list about some false assumptions programmers make about time: https://infiniteundo.com/post/25326999628/falsehoods-programmers-believe-about-time |
vcloarec
force-pushed
the
master
branch
5 times, most recently
from
1fed060
to
c948bda
Compare
| double valid_time = parseMetadataTime( iter->second ); | ||
| *time = ( valid_time - mRefTime ) / 3600.0; // input times are always in seconds UTC, we need them back in hours | ||
| DateTime valid_time = DateTime( parseMetadataTime( iter->second ), DateTime::Unix ); | ||
| *time = ( valid_time - mRefTime ); |
There was a problem hiding this comment.
isn't this missing division by /3600?
would be good to have test in test suite to catch these regressions
There was a problem hiding this comment.
tested indirectly here :
Line 59 in c948bda
vcloarec
force-pushed
the
master
branch
2 times, most recently
from
d58728d
to
33e8ecb
Compare
few modifications and corrections
For discussion