313 make logger use file path and line#323
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-module logger bindings and the exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
- Coverage 90.85% 90.74% -0.12%
==========================================
Files 70 70
Lines 2547 2527 -20
==========================================
- Hits 2314 2293 -21
- Misses 233 234 +1 ☔ 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
requested changes
Feb 17, 2026
Contributor
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
Contributor
Author
|
Updated the emphasize-lines for static14 and static15 after the edits |
In child libraries this breaks because logging is partially enabled
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fastcs/logging/_logging.py`:
- Line 93: The computed padding variable message_pad (used when formatting log
messages) can become negative for long logger names; change its calculation in
the logging routine to clamp to a non-negative value (e.g., replace the raw 100
- len(name) with a max(0, 100 - len(name))) so that downstream string formatting
(where message_pad is applied) never receives a negative width and thus avoids
ValueError; update the calculation near the code that defines message_pad and
ensure any dependent formatting uses the adjusted value.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/tutorials/static-drivers.mdsrc/fastcs/control_system.pysrc/fastcs/logging/__init__.pysrc/fastcs/logging/_logging.pysrc/fastcs/tracer.pysrc/fastcs/transports/controller_api.pysrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/fastcs/transports/epics/ca/ioc.py
- docs/tutorials/static-drivers.md
- src/fastcs/control_system.py
GDYendell
force-pushed
the
313-make_logger_use_file_path_and_line
branch
from
3c06731 to
2169b39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fastcs/transports/controller_api.py`:
- Line 11: Remove the unused Tracer import and the module-level tracer instance:
delete the "Tracer" import (symbol Tracer) and remove the module-level
assignment "tracer = Tracer()" so no unused tracer symbol remains in
controller_api.py; ensure no other code in this module references tracer before
committing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/tutorials/static-drivers.mdsrc/fastcs/control_system.pysrc/fastcs/logging/__init__.pysrc/fastcs/logging/_logging.pysrc/fastcs/tracer.pysrc/fastcs/transports/controller_api.pysrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/fastcs/transports/epics/pva/_pv_handlers.py
- docs/tutorials/static-drivers.md
- src/fastcs/transports/epics/ca/ioc.py
- src/fastcs/control_system.py
GDYendell
force-pushed
the
313-make_logger_use_file_path_and_line
branch
from
2169b39 to
35adfbb
Compare
GDYendell
force-pushed
the
313-make_logger_use_file_path_and_line
branch
from
1740483 to
682ce6c
Compare
GDYendell
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Breaking Changes
bind_loggerremoved. Usefrom fastcs.logging import logger, or create a driver logger usinglogger.opt. Seeloggerdocstring for more informationTracerno longer takes a name