-
Couldn't load subscription status.
- Fork 654
adding absolute timestamp to ASC reader #761
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
|
This might need a rebase due to #741 having been merged two days ago. |
|
Was wondering if we can make this optional by a flag passed to LogReader? In most of our use cases, we would actually prefer the current timestamp format which is easier to read and co-relate back with the ascii logs file. Maybe even make the existing implementation the default approach? |
|
@cperkulator Do you plan on having another look at this? |
oops I really apologize. I'll get this rebased and try and wrap it up today |
Co-authored-by: Alexander Bessman <bessman@users.noreply.github.com>
Co-authored-by: Alexander Bessman <bessman@users.noreply.github.com>
still need to update docs, create test, and add option flag
Codecov Report
@@ Coverage Diff @@
## develop #761 +/- ##
===========================================
- Coverage 70.75% 66.51% -4.25%
===========================================
Files 78 78
Lines 7551 7561 +10
===========================================
- Hits 5343 5029 -314
- Misses 2208 2532 +324 |
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.
Would you mind adding one test for each of the modes relative and absolute? That would make your PR really great!
|
@Mergifyio update |
|
Command
|
|
Not sure what is up with codecov but according to tox, everything I added is covered. |
Yeah, just ignore it in this case. It's somewhat broken. |
|
@cperkulator Somehow, there seem to be sudden errors on the develop branch. See here. Do you know what's happening? They are probably related to this PR, right? |
They all passed before the merge right? I'll take a look. |
@felixdivo alright figured it out. Those failing tests create a header in the temporary ASC file: Since this time is before 1970 (and thus not representable by POSIX time) then we get an I'll submit a new PR to catch that error |
|
Okay, sounds plausible. Thanks for figuring it out! |

needed timestamps to be absolute so I added this.
Probably needs some error handling. Will get to it when I have the chance.