Conversation
📝 WalkthroughWalkthroughRefactors logging to stop creating per-module bound loggers via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
- Coverage 90.84% 90.77% -0.07%
==========================================
Files 70 70
Lines 2555 2537 -18
==========================================
- Hits 2321 2303 -18
Misses 234 234 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/tutorials/static-drivers.md (1)
420-421:⚠️ Potential issue | 🟡 MinorFix out-of-range
emphasize-linesspec — line 123 exceeds file length.The Sphinx warning is valid:
/snippets/static14.pyhas only 121 lines, but theemphasize-linesspec includes line 123, which is out of range. Line 116 is valid. Update the spec to remove line 123 or replace it with a valid line number ≤ 121.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tutorials/static-drivers.md` around lines 420 - 421, The literalinclude directive for the static14.py snippet has an out-of-range entry in its emphasize-lines list; edit the emphasize-lines attribute in the literalinclude block to remove the invalid entry (the trailing/last emphasized item) or replace it with a valid line number within the snippet’s actual length so all emphasized lines fall inside static14.py’s range.
🧹 Nitpick comments (1)
src/fastcs/transports/epics/ca/ioc.py (1)
259-262: Inconsistent use oflogger.warning.The analogous check for attribute PV name length (line 148) uses
logger.warning(...), but this command PV check usesprint(...). Consider aligning them for consistent observability.♻️ Suggested fix
- print( - f"Not creating PV for {attr_name} as full name would exceed" - f" {EPICS_MAX_NAME_LENGTH} characters" - ) + logger.warning( + f"Not creating PV for {attr_name} as full name would exceed" + f" {EPICS_MAX_NAME_LENGTH} characters" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/ca/ioc.py` around lines 259 - 262, The code prints a warning using print(...) when the command PV full name would exceed EPICS_MAX_NAME_LENGTH; replace that print with the module logger (logger.warning(...)) to match the earlier attribute PV check and maintain consistent observability, using the same or equivalent message string and including attr_name and EPICS_MAX_NAME_LENGTH; ensure you reference the same logger instance used elsewhere in this module so the log goes to the configured logger instead of stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/tutorials/static-drivers.md`:
- Around line 420-421: The literalinclude directive for the static14.py snippet
has an out-of-range entry in its emphasize-lines list; edit the emphasize-lines
attribute in the literalinclude block to remove the invalid entry (the
trailing/last emphasized item) or replace it with a valid line number within the
snippet’s actual length so all emphasized lines fall inside static14.py’s range.
---
Nitpick comments:
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 259-262: The code prints a warning using print(...) when the
command PV full name would exceed EPICS_MAX_NAME_LENGTH; replace that print with
the module logger (logger.warning(...)) to match the earlier attribute PV check
and maintain consistent observability, using the same or equivalent message
string and including attr_name and EPICS_MAX_NAME_LENGTH; ensure you reference
the same logger instance used elsewhere in this module so the log goes to the
configured logger instead of stdout.
CI was failing the sphinx build as an emphasized line didn't exist anymore after the changes to logging
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/tutorials/static-drivers.md (3)
451-452:⚠️ Potential issue | 🟡 MinorFix unmatched backtick and sentence wrap.
The inline code forpoweris missing a closing backtick, which breaks formatting.✅ Suggested fix
-Enable tracing on the `power` attribute by calling `enable_tracing` and then enable a -ramp so that the value updates. Check the console to see the messages. Call `disable_tracing` to disable the log messages for `power. +Enable tracing on the `power` attribute by calling `enable_tracing` and then enable a +ramp so that the value updates. Check the console to see the messages. Call `disable_tracing` to disable the log messages for `power`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tutorials/static-drivers.md` around lines 451 - 452, The documentation line misses a closing backtick for the inline code `power` and has a sentence wrap issue; update the sentence to close the inline code (e.g., `power`) and ensure the sentence reads smoothly—call enable_tracing and then enable a ramp so the value updates, check the console for messages, and call disable_tracing to stop logging for `power`—referencing the symbols enable_tracing, disable_tracing, and the power attribute to locate and correct the text.
434-435:⚠️ Potential issue | 🟡 MinorFix grammar in Tracer description.
“adds a enables” reads as a typo and is confusing.Suggested edit: “This adds a
TRACElevel that is disabled by default, but can be enabled at runtime.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tutorials/static-drivers.md` around lines 434 - 435, Fix the grammatical typo in the Tracer description sentence by replacing "This adds a enables logging `TRACE` level log messages that are disabled by default, but can be enabled at runtime." with the suggested clearer wording; update the sentence in the Tracer/AttributeIO documentation to read: "This adds a `TRACE` level that is disabled by default, but can be enabled at runtime." Ensure the change is applied to the Tracer description near the mention of AttributeIO.
472-475:⚠️ Potential issue | 🟡 MinorFix subject–verb agreement.
“These log messages includes” should be “include.”Suggested edit: “These log messages include other trace loggers…”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tutorials/static-drivers.md` around lines 472 - 475, Change the incorrect verb form "includes" to "include" in the sentence that begins "These log messages includes other trace loggers..." so it reads "These log messages include other trace loggers..." (update the phrase "These log messages includes other trace loggers that log messages with `power` as the `topic`" to use "include").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/tutorials/static-drivers.md`:
- Around line 451-452: The documentation line misses a closing backtick for the
inline code `power` and has a sentence wrap issue; update the sentence to close
the inline code (e.g., `power`) and ensure the sentence reads smoothly—call
enable_tracing and then enable a ramp so the value updates, check the console
for messages, and call disable_tracing to stop logging for `power`—referencing
the symbols enable_tracing, disable_tracing, and the power attribute to locate
and correct the text.
- Around line 434-435: Fix the grammatical typo in the Tracer description
sentence by replacing "This adds a enables logging `TRACE` level log messages
that are disabled by default, but can be enabled at runtime." with the suggested
clearer wording; update the sentence in the Tracer/AttributeIO documentation to
read: "This adds a `TRACE` level that is disabled by default, but can be enabled
at runtime." Ensure the change is applied to the Tracer description near the
mention of AttributeIO.
- Around line 472-475: Change the incorrect verb form "includes" to "include" in
the sentence that begins "These log messages includes other trace loggers..." so
it reads "These log messages include other trace loggers..." (update the phrase
"These log messages includes other trace loggers that log messages with `power`
as the `topic`" to use "include").
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
I think all of the highlighted lines in the snippets are going to be wrong because of this change. That can be tested by serving the docs locally with tox -e docs-autobuild
| <level>[{time} {record["level"].name[0]}]</level> \ | ||
| {record["message"]:<80} \ | ||
| {record["message"]} \ | ||
| {f"[{record['file'].path}:{record['line']}]":<80} \ |
There was a problem hiding this comment.
The padding (:<80) should be on the message, not the file path / line.
2 participants
This address #313
Made use of file path and line number in the format record string, then purged the files of bind_logger. Where logging is used in the file, ensured that logger is imported instead of bind_logger
Summary by CodeRabbit
New Features
Breaking Changes
Documentation