Skip to content

Conversation

@mikkeldamsgaard
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed The contributors have signed the CLA label Dec 4, 2023
Copy link
Member

@floitsch floitsch left a comment

Choose a reason for hiding this comment

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

LGTM!
I will ask Kasper or Erik to also have a look.

--name/string?=null
--level/int?=null
--tags/Map?=null
--timestamp/bool?=null:
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be called timestamped.
I think I would change every instance of timestamp with timestamped.

Adds selected attributes to a copy of this logger.
If $name is provided, the $name is added to the copy.
If $level is provied, the $level is added to the copy.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If $level is provied, the $level is added to the copy.
If $level is provided, the $level is added to the copy.

buffer ::= buffer_

if timestamp:
buffer.write "$(Time.monotonic-us / 1000)"
Copy link
Member

Choose a reason for hiding this comment

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

The digits should be padded. Space is probably enough, but 0 would work too.
Not sure what the correct digit-amount is.

/**
Creates a copy where timestamps are enabled.
*/
with-timestamp -> Logger:
Copy link
Member

Choose a reason for hiding this comment

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

I could be convinced that with-timestamps is better than with-timestamped.

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

Labels

cla-signed The contributors have signed the CLA

Development

Successfully merging this pull request may close these issues.

2 participants