Skip to content

Conversation

@ExpandingMan
Copy link
Contributor

For a moment I got myself pretty confused why I could not compose FileLogger and FormatLogger.

This PR explains that to write to a file in a specific format, you should use FormatLogger not FileLogger and adds a method which takes a filename instead of an IO.

In the long run, FileLogger probably ought to be deprecated in favor of FormatLogger and some reasonable defaults.

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

Thanks, I have been thinking about this change for quite some time too.

ExpandingMan and others added 4 commits April 27, 2022 11:58
Co-authored-by: Fredrik Ekre <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
@oxinabox
Copy link
Member

I kind of what to write a IO subtype that is FastFlushing<:IO that wraps another IO and overloads write to always call flush.
Which would then deprecate the FileLogger entirely infavor of SimpleLogger(FastFlushing(open("myfile.log", "a")))
and would deprecate the always_flush argument from FormatLogger.
However that requires the user opening the stream themselves.

The relevance to this PR is that this this pushes the other way.
Making the constructor take the same arguments as open and just pass them on.
Is there a particular reason top prefer
FormatLogger("myrecord.log") over FormatLogger(open("myrecord.log", "a"))?
It is a bit shorter that is for sure
OTOH FormatLogger("myrecord.log", append=false) is similar in length to FormatLogger(open("myrecord.log", "w")).

I don't have super strong opinions about this though, (or any other logging sinks.)
So if @fredrikekre thinks this is good, then i am not going to block it.

@fredrikekre you should merge this, yes?

@fredrikekre fredrikekre merged commit 396afbb into JuliaLogging:master May 13, 2022
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.

3 participants