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

Fix anchorlinks for logs with low-resolution timestamps #15

Closed
wants to merge 1 commit into from

Conversation

kanzure
Copy link

@kanzure kanzure commented Sep 19, 2016

Logs that have many messages in the same minute have broken anchorlinks when the timestamp resolution is minute-level instead of millisecond-level. Here, I use a counter which is appended to each anchorlink.

The unit tests have not been updated yet, so they are presently not passing.

c6855e3

Use a counter and append the value of the counter to each anchorlink in
the generated HTML output. Timestamps from logs with only
minute-resolution will have broken anchorlinks without this fix, because
multiple log lines correspond to a single anchorlink when messages were
sent during the same minute.
@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage decreased (-7.4%) to 92.617% when pulling c6855e3 on kanzure:fix-anchorlinks into b0b1ecb on mgedmin:master.

@kanzure
Copy link
Author

kanzure commented Sep 19, 2016

Also, this breaks previous anchorlinks. It would be better to add an additional <a name="...." /> element instead of repurposing the original <td id="...."> element. This way, previous anchorlinks will continue to function for users that made links to old logs that might get regenerated using these additions.

@mgedmin
Copy link
Owner

mgedmin commented Sep 20, 2016

Another thing I wasn't very happy about was that my anchors were excessively long. Having a counter presents an opportunity to make them shorter. What about using just #m1, #m2, ... instead of #t<fulltimestamp>-<counter>?

@mgedmin
Copy link
Owner

mgedmin commented Sep 25, 2016

I've fixed this in a different way in f8adbe9.

@mgedmin mgedmin closed this Sep 25, 2016
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.

None yet

3 participants