helix-term: add config option for specifying log file #566
Conversation
Signed-off-by: Dmitry Sharshakov <[email protected]>
dsseng
force-pushed
the
log-file-config
branch
from
e86c825
to
a7a4608
Compare
There was a problem hiding this comment.
So I've been holding off on this because I'm not sure if this is something that's really needed and can cause confusion if the user changed the path and forgot. I kind of like having one canonical location. I'm not too decided on this so let's get some more input from others.
| pub struct Config { | ||
| pub theme: Option<String>, | ||
| pub log_file: Option<String>, |
There was a problem hiding this comment.
I would call this log-path rather than log file. You can also use Option<PathBuf> here to parse straight into a PathBuf.
| Err(err) if err.kind() == std::io::ErrorKind::NotFound => Config::default(), | ||
| Err(err) => return Err(Error::new(err)), | ||
| }; | ||
| let log_dir = log_path.parent().unwrap(); |
There was a problem hiding this comment.
Should be .context("unable fetch parent path")?
|
Yes, moving log out and forgetting about it won't be good for support, but how to disable log? It often gets flooded with messages on default settings. |
|
In general I'm in favor of things like this being configurable, but I also think it's probably best to leave it non-configurable until someone has a concrete use-case for it. Knowing why someone needs/wants to change the location will help in deciding how it should be implemented (e.g. should it be in the config vs environment variable vs command-line flag, does it need to be completely disabled in some cases, etc. etc. etc.). So my suggestion is we drop this for now, and if someone comes along in the future with a concrete need/desire for it, we can implement it then. |
|
Well, I made this for the exact use case, so my primary build of Helix I use for editing everything includes this patch. Once I had terminated rust-analyzer by accident from task manager and got 500+ MB of log data with some errors. Then I've decided to be able to drop logs unless debugging stuff. Anyway, if a bug is present user can restart Helix after re-enabling logs and collect everything necessary for report. |
|
Note: in your current implementation if you leave the logfile unset it doesn't actually disable logs but falls back to the system log location. I think to solve log bloat we should raise the default log level to error, then make sure to only print errors which are actually useful for debugging. Setting a max logfile size can work too |
Probably loglevel rise would be a good fix, thank you. As for now I didn't implement disabling by this PR, but can do it if PR is planned to go in. I disable it by redirecting to |
|
Dropping this for now, we'll add it if it becomes necessary -- we log far less now without the |
No description provided.