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

Fix quickstart log4rs.yaml sample #332

Merged
merged 4 commits into from Dec 17, 2023
Merged

Fix quickstart log4rs.yaml sample #332

merged 4 commits into from Dec 17, 2023

Conversation

vladimirr9
Copy link
Contributor

@vladimirr9 vladimirr9 commented Dec 10, 2023

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
Copy link
Owner

estk commented Dec 11, 2023

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

@bconn98 can you review this please?

@bconn98
Copy link
Collaborator

bconn98 commented Dec 14, 2023

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

@vladimirr9 Are you able to add a test?

@vladimirr9
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

bconn98 commented Dec 16, 2023

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
Copy link
Collaborator

bconn98 commented Dec 16, 2023

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
Copy link
Collaborator

bconn98 commented Dec 16, 2023

New issue: #334

estk
estk previously approved these changes Dec 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@estk
Copy link
Owner

estk commented Dec 17, 2023

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

@bconn98
Copy link
Collaborator

bconn98 commented Dec 17, 2023

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

@estk
Copy link
Owner

estk commented Dec 17, 2023

Sounds good to me.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 17, 2023

@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
Copy link
Contributor Author

Made the changes.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 17, 2023

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

@bconn98
Copy link
Collaborator

bconn98 commented Dec 17, 2023

@vladimirr9 Actually looks like you need to rebase main in

@vladimirr9
Copy link
Contributor Author

Rebased

@estk estk merged commit 58b92c8 into estk:main Dec 17, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants