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

[Feature Request] Event handlers for FileSink and RotatingFileSink #53

Open
1 task
SpriteOvO opened this issue Dec 13, 2023 · 5 comments
Open
1 task
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@SpriteOvO
Copy link
Owner

SpriteOvO commented Dec 13, 2023

Implement event handlers (reference from C++ spdlog) for FileSink and RotatingFileSink by adding new methods on the builders of these file sinks.

The names of these methods are tentatively designated as before_file_open, after_file_open, before_file_close and after_file_close.

Unsolved questions:

  • Should we pass a &mut PathBuf to before_file_open handler to allow users to change the file path?
@SpriteOvO SpriteOvO added enhancement New feature or request good first issue Good for newcomers labels Dec 13, 2023
@Lancern
Copy link
Collaborator

Lancern commented Dec 13, 2023

Should we pass a &mut PathBuf to before_file_open handler to allow users to change the file path?

I don't think this is necessary since you can change the path via FileSinkBuilder::path. A &Path shoud suffice.

@SpriteOvO
Copy link
Owner Author

SpriteOvO commented Dec 13, 2023

I don't think this is necessary since you can change the path via FileSinkBuilder::path. A &Path shoud suffice.

But for RotatingFileSink, a &mut PathBuf allows the user to customize the rotated file name before creating the file, rather than renaming it after the file is closed. (context #49)

I'm thinking if there are any cons to doing it this way.

@Lancern
Copy link
Collaborator

Lancern commented Dec 14, 2023

The reason I feel a bit strange is that if you give your user a &mut PathBuf in the hook, it strongly expresses the intent that the hook exists solely for a very specific purpose: changing the filename. To implement this very specific requirement, a better API might be:

pub trait RotateFileOpener {
    fn open(&self /*, other params */) -> PathBuf;
}

impl<ArgBP, ArgRP> RotateFileSinkBuilder<ArgBP, ArgRP> {
    pub fn opener(self, opener: Box<dyn RotateFileOpener>) -> Self {
        todo!()
    }
}

The hook can be kept (and it should be kept). It's purpose is to give the user a change to do monitoring and bookkeeping jobs. But I still don't think changing the filename from the hook is a good idea. We have other approaches to express the indent more clearly.

@SpriteOvO
Copy link
Owner Author

SpriteOvO commented Dec 14, 2023

The reason I feel a bit strange is that if you give your user a &mut PathBuf in the hook, it strongly expresses the intent that the hook exists solely for a very specific purpose: changing the filename.

Makes sense. So let's keep &Path for before_file_open of RotatingFileSink.

The new trait RotateFileOpener is a bit verbose if it's just for custom paths, and it should be designed to be a bit more powerful if it goes this way. Another option would be to just add something like the filename_calculator in C++ spdlog. But for now let's table this idea, it seems a bit out of the scope of this issue. If anyone is interested in this in the future we will discuss it further in a new issue. For users who currently need to customize file names, they can workaround renaming files in after_file_close.

So the current signatures of the event handlers will be
(reference from C++ spdlog)

before_file_open  : impl Fn(path: &Path)
after_file_open   : impl Fn(path: &Path, file: &mut File)
before_file_close : impl Fn(path: &Path, file: &mut File)
after_file_close  : impl Fn(path: &Path)

Is it right?

@Lancern
Copy link
Collaborator

Lancern commented Dec 14, 2023

So the current signatures of the event handlers will be
[...]
Is it right?

What's the purpose of the &mut File parameter? This would allow the user to replace the File object completely through the mutable reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants