Feature/sub hourly #85
Conversation
|
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #85 +/- ##
===========================================
+ Coverage 62.80% 63.03% +0.22%
===========================================
Files 790 791 +1
Lines 45196 45338 +142
===========================================
+ Hits 28386 28577 +191
+ Misses 16810 16761 -49 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The changes here are all an improvement 😄 👍
I suggest in the future (because now isn't the right time) to move this class downstream, as it is not at the righ place on the pgen/mars-client stack (to my knowledge); it isn't "down enough", it is part of, and contains, MARS-specific interpretation of time, and should exist at the language parsing level instead.
src/eckit/types/Time.cc
Outdated
| if (seconds >= 86400 || seconds < 0) { | ||
| std::string msg = "Time in seconds must be positive and cannot exceed 86400, seconds: "; | ||
| if ((seconds >= 86400 && !extended) || seconds < 0) { | ||
| std::string msg = "Time in seconds must be positive and less than 86400 seconds (24h): "; | ||
| Translator<long, std::string> t; |
There was a problem hiding this comment.
Convert Translator<long, std::string> to std::to_string
| @@ -8,6 +8,9 @@ | |||
| * does it submit to any jurisdiction. | |||
| */ | |||
|
|
|||
| #include <regex> | |||
| #include <cmath> | |||
|
|
|||
| #include "eckit/eckit.h" | |||
|
|
|||
| #include "eckit/persist/DumpLoad.h" | |||
There was a problem hiding this comment.
Remove #include "eckit/utils/Tokenizer.h"
src/eckit/types/Time.cc
Outdated
| err = true; | ||
| break; | ||
| Time::Time(const std::string& s, bool extended) { | ||
| long ss = 0, mm = 0, hh = 0, dd = 0; |
There was a problem hiding this comment.
Declarations should be one-per-line
| @@ -32,9 +32,9 @@ typedef double Second; | |||
| class Time { | |||
There was a problem hiding this comment.
Modernise
typedef double Second
to
using Second = double;
src/eckit/types/Time.h
Outdated
| Time(const std::string&); | ||
| Time(long, long, long, bool extended = false); | ||
| Time(long seconds = 0, bool extended = false); | ||
| Time(const std::string&, bool extended = false); |
There was a problem hiding this comment.
Missing the arguments names (since they are not deducible by the type).
To be consistent, it's also missing one constructor
Time(long minutes, long seconds, bool extended = false);
| std::string msg = "Wrong input for time: "; | ||
| Translator<long, std::string> t; | ||
| if (dd>0) { |
There was a problem hiding this comment.
This isn't consistent with the check above, for ss<86400. (eg, line 35 but there are more). If the class isn't to support days, and it also doesn't have and accessor for it, this should actually fail (or, remove the checks and support it)
There was a problem hiding this comment.
Also, this printing of time as string is repeated in multiple places -- ideally, these places should be replaced with a operator std::string (that you already have)
src/eckit/types/Time.cc
Outdated
| @@ -27,80 +30,98 @@ inline void printTime(std::ostream& s, long n) { | |||
| s << n; | |||
| } | |||
|
|
|||
| Time::Time(long seconds) : | |||
| Time::Time(long seconds, bool extended) : | |||
| seconds_(seconds) { | |||
There was a problem hiding this comment.
This requires a seconds_(static_cast(seconds))
| EXPECT(Time(0,3,0) == Time("3M")); | ||
| EXPECT(Time(0,3,0) == Time("180s")); | ||
|
|
||
| EXPECT(Time(1,23,45) == Time("1h23m45s")); |
There was a problem hiding this comment.
We ought to have tests that cover such things as 1h2m3s (I.e. where mins and secs are single digit, including following previous terms).
| seconds_(hh * 3600 + mm * 60 + ss) { | ||
| if (hh >= 24 || mm >= 60 || ss >= 60 || hh < 0 || mm < 0 || ss < 0) { | ||
| if (mm >= 60 || ss >= 60 || (!extended && (hh >= 24 || hh < 0 || mm < 0 || ss < 0))) { |
There was a problem hiding this comment.
IS there a reason we can't don't support 61 minutes, or 61 seconds, and lroll them into larger time units, given that we do that with hours here?
No description provided.