Add option to add timestamps to the log output #11
Conversation
|
Closes #10 |
|
Thank you for the contribution! We're about to take a closer look at all the feedback we've gotten so far to consider the API design for our next version. |
| @@ -1,4 +1,5 @@ | |||
| target/ | |||
| Cargo.lock | |||
There was a problem hiding this comment.
I haven't seen this before, but just found the following resource on removing Cargo.lock
Is this change for the same rationale?
https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries
There was a problem hiding this comment.
I didn't know it is recommended to check in the .lock file for binaries, I never checked it in. But it works either way and in general, the Cargo.lock is not checked in. But apparently it can be used to determine the exact dependencies a build was compiled with.
There was a problem hiding this comment.
Actually if you don't check in the Cargo.lock for a binary, the build will in general not be repeatable, because the versions of dependencies could have been updated. If you have a dependency with dependency = "x.y.z" in your Cargo.toml, cargo will take any version that has the same major number x.y and at least z minor version. You will need to specify "=x.y.z to get the exact version, however this is rarely done.
| @@ -23,3 +23,4 @@ crate-type = ["cdylib", "rlib"] | |||
| tracing = {version = "0.1", features = ["attributes"]} | |||
| tracing-subscriber = {version = "0.2", features = ["registry"]} | |||
| wasm-bindgen = {version = "0.2"} | |||
| chrono = { version = "^0.4", features = ["wasmbind"] } | |||
There was a problem hiding this comment.
I've seen chrono often put behind a feature flag. Is that possibly to allow avoiding extra dependencies?
We might consider adding a feature-flag like that here as a follow-up to this PR.
There was a problem hiding this comment.
I am not sure about that, chrono was once incompatible with wasm, which had already caused me issues, but is fixed now apparently. It may probably still be useful to have a feature flag for time logging.
There was a problem hiding this comment.
Being able to avoid extra dependencies is always a good thing, potentially leading to shorter compile times and less network bandwidth used for the build. On the other hand, every feature carries a complexity cost, and unfortunately features aren't that discoverable yet. As chrono is fairly small and very compatible (depending on features, it can even be no_std), I don't think we'll need to put it behind a feature. I don't understand the usage sufficiently to gauge if we should at least make wasmbind a default feature otherwise.
src/lib.rs
Outdated
| let timestamp = if self.config.log_time { | ||
| chrono::Local::now().format("%a %d %T%.f").to_string() | ||
| } else { | ||
| String::new() | ||
| }; |
There was a problem hiding this comment.
If we're in certain WASM contexts where Date is available, I wonder if we could just leverage (new Date()).toSomeFormatString(). This may allow us to drop chrono entirely.
Just thinking out loud.
@llogiq might have other ideas.
There was a problem hiding this comment.
there is js_sys which exposes the Date API. Although I think it is not very future proof, as most Rust crates use std::time or chrono. I think js_sys is even used internally by chrono to get the time in wasm.
There was a problem hiding this comment.
As I wrote above, I doubt that chrono is increasing our compile times too much, and with javascript methods I'm always wary about the implementation (though nowadays runtimes are usually very well implemented, perhaps that's just the cranky old man in me speaking 😄).
This file should not be pushed to git
|
Sorry I forgot about this PR. Resolved the conflicts. |
The timestamps currently have a fixed formatting, but it should be fine for most cases and matches the default format in tracing.
It should be pretty easy to add configuration for this later on but I favored simplicity for now.