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 CR LF definition #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix CR LF definition #27

wants to merge 1 commit into from

Conversation

ldenisey
Copy link

Hello,

Thank you for maintaining the lib, it is very handy !

Sorry if I missed something but it seems that CR and LF definition were inverted.

CR and LF definition were inverted
@lbussy
Copy link

lbussy commented Aug 24, 2023

While this is technically correct, the historical examples gave CR as the line terminator. This change breaks that behavior and, in PIO for instance, leads to the same line being overwritten endlessly.

@egnor
Copy link

egnor commented Apr 16, 2024

Maybe change the examples to use "\n" instead of CR, but keep CR (defined to be LF!) for compatibility with a warning comment next to its definition? (It's also a little antisocial to be defining an unqualified two letter symbol like that, but that's another matter...)

But it's kind of weird that CR/LF doesn't get added automatically-- is the idea that you could emit multiple log events to construct a single longer line? That doesn't work with prefixes, though. Honestly I'd suggest to strip any trailing CR/LF characters from log text, add CR/LF after the suffix always, make the .xxxln() calls trivial passthrough to .xxx(), and drop all the newline-related stuff from the documentation. This is what almost every other logging library in the universe does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants