-
Notifications
You must be signed in to change notification settings - Fork 52
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
[#4] Introduce developer guide with log documentation #469
[#4] Introduce developer guide with log documentation #469
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #469 +/- ##
==========================================
- Coverage 79.08% 78.95% -0.13%
==========================================
Files 196 197 +1
Lines 23572 23637 +65
==========================================
+ Hits 18641 18663 +22
- Misses 4931 4974 +43 |
f3da883
to
86fc0e5
Compare
86fc0e5
to
3f4dd87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
0 [T] FileBuilder { file_path: FilePath { value: FixedSizeByteString<255> { | | ||
len: 20, data: "config/iceoryx2.toml" } }, access_mode: Read, | | ||
permission: Permission(448), has_ownership: false, owner: None, | | ||
group: None, truncate_size: None, creation_mode: None } | | ||
| opened ---------------------------------------------------------------+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR but would it be possible to have the actual log message before the origin?
I volunteer to change this, if you agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? For me it feels wrong but if @orecham also prefers to switch the output order of the console logger I would go with the flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it feels like yoda notation 😅
At first it's the log level, e.g. [W]
. Then it's the context, but for what? Finally, it's the actual log message and the context becomes meaningful but one has to scroll back to the top.
I would even change the format to this in order to have a clean separation between log message and context
0 [T] opened
=== Context ===
FileBuilder { file_path: FilePath { value: FixedSizeByteString<255> { |
len: 20, data: "config/iceoryx2.toml" } }, access_mode: Read, |
permission: Permission(448), has_ownership: false, owner: None, |
group: None, truncate_size: None, creation_mode: None }
Instead of ### Context ###
something like ========
would also work.
Notes for Reviewer
A detailed documentation about logging (and also how to handle errors since they are tightly coupled to logging) in iceoryx2.
Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MIT
iox2-123-introduce-posix-ipc-example
)[#123] Add posix ipc example
)task-list-completed
)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Relates to #4