Skip to content

Conversation

@cperkulator
Copy link
Contributor

needed timestamps to be absolute so I added this.

Probably needs some error handling. Will get to it when I have the chance.

@cperkulator cperkulator requested a review from felixdivo January 29, 2020 20:27
@felixdivo
Copy link
Collaborator

This might need a rebase due to #741 having been merged two days ago.

@sou1hacker
Copy link
Contributor

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?

@felixdivo
Copy link
Collaborator

@cperkulator Do you plan on having another look at this?

@cperkulator
Copy link
Contributor Author

cperkulator commented Apr 23, 2021

@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

Caleb Perkinson and others added 4 commits April 23, 2021 12:06
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
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #761 (7389b23) into develop (4048765) will decrease coverage by 4.24%.
The diff coverage is 100.00%.

@@             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     

Copy link
Collaborator

@felixdivo felixdivo left a 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!

@felixdivo felixdivo added enhancement file-io about reading & writing to files labels Apr 23, 2021
@felixdivo
Copy link
Collaborator

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 23, 2021

Command update: success

Branch already up to date

@cperkulator
Copy link
Contributor Author

Not sure what is up with codecov but according to tox, everything I added is covered.

@felixdivo
Copy link
Collaborator

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.

@felixdivo felixdivo removed the request for review from hardbyte April 23, 2021 21:27
@mergify mergify bot merged commit 0bcbf45 into hardbyte:develop Apr 23, 2021
@mergify mergify bot requested a review from hardbyte April 23, 2021 21:27
@felixdivo
Copy link
Collaborator

@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?

@cperkulator
Copy link
Contributor Author

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

@cperkulator
Copy link
Contributor Author

@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:
image

Since this time is before 1970 (and thus not representable by POSIX time) then we get an OSError exception.

I'll submit a new PR to catch that error

@felixdivo
Copy link
Collaborator

Okay, sounds plausible. Thanks for figuring it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement file-io about reading & writing to files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants