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.

Sign up for GitHub

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

Jump to bottom

feat: add log filter #98

Merged
merged 5 commits into from
Aug 19, 2023
Merged

feat: add log filter #98

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c4fbd1
feat: use newest logid with filter
mhatzl Jul 22, 2023
a793606
Merge branch 'main' into log-filter
mhatzl Jul 22, 2023
f11ba84
fix: remove string references for log msgs
mhatzl Jul 22, 2023
fe3ed0a
fix: set stderr as log output for cli test
mhatzl Jul 22, 2023
2a3b807
feat: change to logid filter builder
mhatzl Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ readme = "README.md"
license = "MIT"

[workspace.dependencies]
logid = { version = "0.9", features = ["diagnostics"] }
logid = { version = "0.11", features = ["diagnostics"] }
thiserror = "1.0"
once_cell = "1.13.0"
clap = { version = "4.2.7", features = ["derive", "cargo", "env"] }
Expand Down
6 changes: 3 additions & 3 deletions cli/src/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub fn compile(config: Config) -> Result<(), GeneralError> {
let source = fs::read_to_string(&config.input).map_err(|error| {
pipe!(
GeneralError::FileRead,
&format!("Could not read file: '{:?}'", &config.input),
format!("Could not read file: '{:?}'", &config.input),
add: AddonKind::Info(format!("Cause: {}", error))
)
})?;
Expand Down Expand Up @@ -66,13 +66,13 @@ fn write_file(

log!(
GeneralInfo::WritingToFile,
&format!("Writing to file: {:?}", full_out_path),
format!("Writing to file: {:?}", full_out_path),
);

std::fs::write(&full_out_path, content).map_err(|error| {
pipe!(
GeneralError::FileWrite,
&format!("Could not write to file: {:?}", full_out_path),
format!("Could not write to file: {:?}", full_out_path),
add: AddonKind::Info(format!("Cause: {}", error))
)
})
Expand Down
12 changes: 6 additions & 6 deletions cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clap::Parser;
use logid::{event_handler::LogEventHandlerBuilder, log, logging::event_entry::AddonKind};
use logid::{event_handler::builder::LogEventHandlerBuilder, log, logging::event_entry::AddonKind};
use unimarkup_commons::config::{Config, ConfigFns};

use crate::log_id::{GeneralError, GeneralInfo};
Expand All @@ -8,8 +8,10 @@ mod compiler;
mod log_id;

fn main() {
let log_handler = LogEventHandlerBuilder::new()
.write_to_console()
let _ = logid::set_filter!("info(infos)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I ask why is this a macro? Looking at the source code I feel like it could (and should) be a function. Also, would it be possible to use enums for filter level? Using string feels very error prone to be honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to have an environmental variable to set the filter, but the variable would need to be set during compiletime, and I was not able to get this to work using environmental variables in cargo's config.toml file.
With the string approach, setting the filter is the same for the environmental variable or via macro/function.

I also just wanted to get the filter working to have at least some filter.

The macro could be a function, but this would be a change in logid, and this might change in the future if I add another way to set the filter without using the string syntax.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea was to have an environmental variable to set the filter, but the variable would need to be set during compiletime

Is there a reason you need the environment variable at the compile time? I would expect that to be something we can change after the compilation (at runtime). For example, if user reports an issue, we can instruct them to set the corresponding environment variable so that the compiler reports more information that can be useful for us.

The macro could be a function, but this would be a change in logid, and this might change in the future

Justification like this one is often problematic, because we don't know when that future will be. I think that if we can improve it now, we should do it now. The future change might be very soon, but it might also be next year, or maybe even never.

That being said, even if the macro signature will change (e.g. accepts type instead of literal), then it could still be a function. And if there should be other ways to construct a filter, I would prefer multiple functions over overloading a single macro.

Also, Macro heavy code is often harder to understand and increases compilation time. If we don't need features of macro (e.g. code to expand, or the function to be variadic or use custom syntax etc.), then it could probably be just a regular function. This looks like a candidate for me that should be reduced from macro to a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason you need the environment variable at the compile time? I would expect that to be something we can change after the compilation (at runtime). For example, if user reports an issue, we can instruct them to set the corresponding environment variable so that the compiler reports more information that can be useful for us.

It is needed at compilation time, because the logger is setup at compilation time, and so is the initial filter.
As I do not want to check if the environmental variable changed on every log entry, the environmental variable must be available at compile time to be used for the initial filter.

Justification like this one is often problematic, because we don't know when that future will be. I think that if we can improve it now, we should do it now. The future change might be very soon, but it might also be next year, or maybe even never.

Even if the change might not come in the next months, it only affects one line in Unimarkup, so I prefer to keep it as is for now, and use logid more to better understand what we need for filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to newest logid version that offers a filter builder instead of the set_filter macro.

filter builder was implemented by @nfejzic. Thank you :)


let _handler = LogEventHandlerBuilder::new()
.to_stderr()
.all_log_events()
.build();

Expand All @@ -19,7 +21,7 @@ fn main() {
if let Err(err) = cfg.validate() {
logid::log!(
GeneralError::Compile,
&format!("Configuration is invalid: {err}")
format!("Configuration is invalid: {err}")
);
break 'outer;
}
Expand All @@ -46,6 +48,4 @@ fn main() {
}
}
}

log_handler.shutdown();
}
19 changes: 11 additions & 8 deletions cli/tests/logging/cli_logs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,23 @@ fn test__main_log_trace__attributes_file() {
path.push(format!("tests/test_files/{}", TEST_FILE));

let cli_proc = Command::new("cargo")
.stdout(Stdio::piped())
.args(["run", "--", "--formats=html", &path.to_string_lossy()])
.stderr(Stdio::piped())
.args([
"run",
"--",
"--formats=html",
"--lang=en",
&path.to_string_lossy(),
])
.spawn()
.expect("Failed to spawn 'cargo run'");

let output = cli_proc
.wait_with_output()
.expect("Failed to execute 'cargo run'");
let logs = String::from_utf8_lossy(&output.stdout);
let logs = String::from_utf8_lossy(&output.stderr);

assert!(logs.contains("INFO: Writing to file: "));
assert!(logs.contains("Writing to file: "));
assert!(logs.contains(&TEST_FILE.replace(".um", ".html")));
// assert!(logs.contains("64(origin): file="));
assert!(logs.contains("INFO: Unimarkup finished compiling."));
// assert!(logs.contains(TEST_FILE));
// assert!(logs.contains("65(origin): file="));
assert!(logs.contains("Unimarkup finished compiling."));
}
4 changes: 2 additions & 2 deletions commons/src/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl ConfigFns for Config {
if !self.input.exists() {
return err!(
ConfigErr::InvalidFile,
&format!("Input file not found: {:?}", self.input)
format!("Input file not found: {:?}", self.input)
);
}

Expand All @@ -90,7 +90,7 @@ impl Config {
.map_err(|_| {
pipe!(
ConfigErr::InvalidFile,
&format!(
format!(
"Failed to read locales file: {:?}",
locales_file.as_ref().map(|p| p.to_string_lossy())
)
Expand Down
2 changes: 1 addition & 1 deletion commons/src/config/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl ConfigFns for Output {
if filepath.exists() {
return err!(
ConfigErr::InvalidConfig,
&format!(
format!(
"Output file '{:?}' already exists, but `overwrite` was not set.",
filepath
)
Expand Down
20 changes: 10 additions & 10 deletions commons/src/config/preamble.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl ConfigFns for I18n {
{
return err!(
ConfigErr::BadLocaleUsed,
&format!(
format!(
"{} locale(s) not supported by default. Only the following locales are allowed: {}.",
locales
.iter()
Expand Down Expand Up @@ -125,7 +125,7 @@ impl ConfigFns for I18n {
.map_err(|_| {
pipe!(
ConfigErr::InvalidFile,
&format!(
format!(
"Failed to read locales file: {}",
self.locales_file.as_ref().unwrap().to_string_lossy()
)
Expand Down Expand Up @@ -159,7 +159,7 @@ impl ConfigFns for I18n {
provider.load_buffer(key, req).map_err(|err| {
pipe!(
ConfigErr::BadLocaleUsed,
&format!(
format!(
"Could not find locale '{}' in data file. Cause: {}",
locale, err
)
Expand Down Expand Up @@ -204,7 +204,7 @@ impl I18n {
Err(err) => {
logid::log!(
ConfigErr::InvalidFile,
&format!(
format!(
"Locales file not found: {}. Cause: {}. Using default locales file.",
file_path.to_string_lossy(),
err
Expand All @@ -229,7 +229,7 @@ impl I18n {
let f = File::create(&file_path).map_err(|_| {
pipe!(
ConfigErr::FileCreate,
&format!(
format!(
"Failed to create locales file: {}",
file_path.as_ref().to_string_lossy()
)
Expand All @@ -246,7 +246,7 @@ impl I18n {
.map_err(|err| {
pipe!(
ConfigErr::LocaleDownload,
&format!(
format!(
"Failed to download locales file: {}. Cause: {}",
file_path.as_ref().to_string_lossy(),
err,
Expand Down Expand Up @@ -302,7 +302,7 @@ impl ConfigFns for Citedata {
if !file.exists() {
return err!(
ConfigErr::InvalidFile,
&format!("Citation Style Language file not found: {:?}", file)
format!("Citation Style Language file not found: {:?}", file)
);
}
}
Expand All @@ -311,7 +311,7 @@ impl ConfigFns for Citedata {
if !reference.exists() {
return err!(
ConfigErr::InvalidFile,
&format!("Bibliography references file not found: {:?}", reference)
format!("Bibliography references file not found: {:?}", reference)
);
}
}
Expand Down Expand Up @@ -348,7 +348,7 @@ impl ConfigFns for Metadata {
if !font.exists() {
return err!(
ConfigErr::InvalidFile,
&format!("Font file not found: {:?}", font)
format!("Font file not found: {:?}", font)
);
}
}
Expand Down Expand Up @@ -384,7 +384,7 @@ impl ConfigFns for HtmlSpecificParameter {
if !fav.exists() {
return err!(
ConfigErr::InvalidFile,
&format!("Favicon file not found: {:?}", fav)
format!("Favicon file not found: {:?}", fav)
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion parser/src/security/hashing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub fn get_filehash(file: &Path) -> Result<Vec<u8>, HashingError> {
let source = fs::read_to_string(file).map_err(|err| {
pipe!(
HashingError::FailedReadingFile,
&format!("Could not read file: '{:?}'", file),
format!("Could not read file: '{:?}'", file),
add: AddonKind::Info(format!("Cause: {}", err))
)
})?;
Expand Down
2 changes: 1 addition & 1 deletion render/src/log_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use logid::{evident::event::intermediary::FinalizedEvent, log_id::LogId, ErrLogId};
use logid::{evident::event::finalized::FinalizedEvent, log_id::LogId, ErrLogId};
use thiserror::Error;

#[derive(Debug, Clone, ErrLogId, Error)]
Expand Down