Skip to content
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

Add support for time as timestamp #21

Merged
merged 3 commits into from
Feb 21, 2018
Merged

Add support for time as timestamp #21

merged 3 commits into from
Feb 21, 2018

Conversation

cedk
Copy link
Contributor

@cedk cedk commented Feb 20, 2018

I use the log from ii which switched to timestamp format for the log.

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate a small example log file using this format, for tests. Something on the order of 5-10 lines, ideally containing regular messages, actions, joins/parts, maybe even a nick change.

Currently irclog2html has 100% test coverage and I do not wish to lose that. Would you be willing to try and write a unit test for your changes? If not, I'll see if I can find the time to do it myself.

@@ -119,6 +120,7 @@ class LogParser(object):
r'(?:\d{4}-\d{2}-\d{2}T|\d{2}-\w{3}-\d{4} |\w{3} \d{2} |\d{2} \w{3} )?' # Optional date
r'\d\d:\d\d(:\d\d)?' # Mandatory HH:MM, optional :SS
r')\]? +') # Optional ], mandatory space
TIMESTAMP_REGEXP = re.compile(r'^(\d*) +')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be \d+, to require at least one digit.

if m:
time = datetime.datetime.fromtimestamp(
int(m.group(1))).isoformat()
line = line[len(m.group(0)):]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Travis failure looks to be caused by timezones. Now I'm wondering if this should be utcfromtimestamp(), or if I should change my test harness to set TZ=UTC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it makes sense to parse timestamp as UTC.

@mgedmin
Copy link
Owner

mgedmin commented Feb 21, 2018

Thank you!

@mgedmin mgedmin merged commit 99cd09c into mgedmin:master Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants