estk / log4rs

A highly configurable logging framework for Rust
Apache License 2.0
973 stars 143 forks source link

Fix quickstart log4rs.yaml sample #332

Closed vladimirr9 closed 7 months ago

vladimirr9 commented 7 months ago

Fails parsing the current config. Seems to have been changed accidentally as it doesn't make sense that root and logger are defined within appenders.

estk commented 7 months ago

Can you add some test that verify that this doesn't happen again?

@bconn98 can you review this please?

bconn98 commented 7 months ago

@estk @vladimirr9 Can confirm the README was incorrect and now works as expected.

@vladimirr9 Are you able to add a test?

vladimirr9 commented 7 months ago

Not too experienced with rust. I see there's already a test to ensure the config is read properly and correct under src/config/raw.rs. This seems to have been a mismatch between documentation and what is actually valid input. Don't know how I'd design a test to ensure that the documentation itself is correct.

vladimirr9 commented 7 months ago

Added a test that does readme parsing in order to get the config from there. Verify if it's OK and if the location is OK.

bconn98 commented 7 months ago

The test does indeed catch that the config in main is bad. However, it doesn't catch a couple other weird indentations I give it. Let me think on it.

bconn98 commented 7 months ago

The failure to capture the bad configs isn't your test lacking but the deserializer failing to capture. @estk IMO this deserves a followup issue to investigate and address.

bconn98 commented 7 months ago

New issue: https://github.com/estk/log4rs/issues/334

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ebb9123) 61.12% compared to head (ed2f885) 61.12%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #332 +/- ## ======================================= Coverage 61.12% 61.12% ======================================= Files 23 23 Lines 1407 1407 ======================================= Hits 860 860 Misses 547 547 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

estk commented 7 months ago

not sure how to resolve the lint stuff, @bconn98 , any ideas?

bconn98 commented 7 months ago

@estk we don’t have many markdown files. We could just reduce the spaces to 2 for markdown as well as yaml.

estk commented 7 months ago

Sounds good to me.

bconn98 commented 7 months ago

@vladimirr9 Hey, can you resolve the rustfmt error by cleaning up the trailing whitespace? Also make the following change in .editorconfig:

[*.md]
trim_trailing_whitespace = false
indent_size = 2
vladimirr9 commented 7 months ago

Made the changes.

bconn98 commented 7 months ago

@estk kick off a CI and then it should be all set

bconn98 commented 7 months ago

@vladimirr9 Actually looks like you need to rebase main in

vladimirr9 commented 7 months ago

Rebased