Pass attribute instead of datatype into _get_read_widget#321
Conversation
📝 WalkthroughWalkthroughRefactor EPICS GUI handlers to accept full Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 #321 +/- ##
=======================================
Coverage 90.83% 90.83%
=======================================
Files 70 70
Lines 2553 2554 +1
=======================================
+ Hits 2319 2320 +1
Misses 234 234 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GDYendell
force-pushed
the
pass_attribute_into_get_read_widget
branch
from
98ddf4f to
3eb02cc
Compare
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
Looks good! Minor comments and as you suggested we should update the write equivalents to match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/fastcs/transports/epics/pva/gui.py (2)
58-58: Semantic mismatch:AttrR(read-only) used to drive write-widget selection.
super()._get_write_widget(AttrR(fastcs_datatype))wraps a column datatype in a read-only attribute solely so the parent's dispatch (attribute.datatype) resolves correctly. This works today becauseEpicsGUI._get_write_widgetignores the attribute kind, but it's misleading and fragile if that assumption changes.A cleaner pattern would be to extract a small private helper that accepts a raw
DataTypedirectly (matching the pre-refactor contract), rather than constructing a semantically incorrectAttrRwrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/pva/gui.py` at line 58, The call that constructs a read-only wrapper AttrR(fastcs_datatype) to feed into super()._get_write_widget is semantically wrong; instead add a small private helper (e.g., _select_write_widget_for_datatype) that accepts the raw DataType and encapsulates the dispatch logic, then change the call site to call this helper with fastcs_datatype and have it delegate to EpicsGUI._get_write_widget or the superclass logic without fabricating an AttrR; update references to AttrR(...) in this file to use the new helper so attribute kind is not faked.
11-12: Inconsistent import style: use package-levelfastcs.attributesimports.The parent module
src/fastcs/transports/epics/gui.pyimports viafrom fastcs.attributes import Attribute, AttrR, .... These two lines reach into private submodules directly, which is inconsistent and couples this file to the internal layout.♻️ Proposed fix
-from fastcs.attributes.attr_r import AttrR -from fastcs.attributes.attribute import Attribute +from fastcs.attributes import AttrR, Attribute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/pva/gui.py` around lines 11 - 12, Imports in gui.py dig into submodules (using "from fastcs.attributes.attr_r import AttrR" and "from fastcs.attributes.attribute import Attribute"), which is inconsistent with package-level imports; change them to use the package-level API by importing AttrR and Attribute from fastcs.attributes (e.g., replace those two specific import statements with a single "from fastcs.attributes import Attribute, AttrR"), keeping the same symbols so other code (references to Attribute and AttrR) remains unchanged and preserving import ordering/style used in the parent module.tests/transports/epics/ca/test_gui.py (1)
96-96: Nitpick: usearray_dtype=keyword argument for consistency.Every other
Waveforminstantiation in this file uses the explicit keyword argumentWaveform(array_dtype=np.int32)(Lines 43, 57), but this line omits the keyword.♻️ Proposed fix
- assert gui._get_write_widget(attribute=AttrR(Waveform(np.int32))) is None + assert gui._get_write_widget(attribute=AttrR(Waveform(array_dtype=np.int32))) is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transports/epics/ca/test_gui.py` at line 96, The assertion uses Waveform(np.int32) without the explicit keyword; update the call to use the same style as the rest of the file by changing Waveform(np.int32) to Waveform(array_dtype=np.int32) in the assertion that calls gui._get_write_widget(attribute=AttrR(Waveform(...))). This keeps instantiation consistent with other lines using Waveform(array_dtype=...) and avoids positional-argument ambiguity for the Waveform constructor.src/fastcs/transports/epics/gui.py (1)
81-81: Optional: move longTypeErrormessage into the exception class or a constant.Ruff (TRY003) flags inline long messages on
raisestatements. Extracting the message to a constant or a dedicated exception subclass would satisfy this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/gui.py` at line 81, The inline long TypeError message in src/fastcs/transports/epics/gui.py should be extracted: either define a module-level constant like UNHANDLED_DATATYPE_MSG = "Unsupported type {type}: {value}" and format it at raise, or create a small subclass UnsupportedDatatypeError(Exception) that accepts the datatype and builds the message; then replace the current raise TypeError(f"...") with raise UnsupportedDatatypeError(datatype) or raise TypeError(UNHANDLED_DATATYPE_MSG.format(type=type(datatype), value=datatype)); reference the existing raise site (the TypeError raise in this module) when making the change.
🤖 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/epics/gui.py`:
- Around line 73-76: The warning currently interpolates waveform.shape (a tuple)
which prints dimensions like "(10, 10)D"; change the interpolation to the
dimensionality count by using waveform.ndim (or len(waveform.shape)) in the
logger.warning call so the message becomes e.g. "... is a 2D waveform" — update
the logger.warning invocation that references attribute and waveform (in this
EPICS CA transport check) to use waveform.ndim instead of waveform.shape.
In `@src/fastcs/transports/epics/pva/gui.py`:
- Around line 47-52: Remove the dead binding `datatype = attribute.datatype` in
the `_get_write_widget` method: it's unused because `match attribute.datatype:`
and the loop `for _, datatype in attribute.datatype.structured_dtype:` shadow
it; delete that assignment so only `attribute.datatype` is referenced via the
match and loop variables (`attribute`, `attribute.datatype`, and loop
`datatype`) remain.
---
Nitpick comments:
In `@src/fastcs/transports/epics/gui.py`:
- Line 81: The inline long TypeError message in
src/fastcs/transports/epics/gui.py should be extracted: either define a
module-level constant like UNHANDLED_DATATYPE_MSG = "Unsupported type {type}:
{value}" and format it at raise, or create a small subclass
UnsupportedDatatypeError(Exception) that accepts the datatype and builds the
message; then replace the current raise TypeError(f"...") with raise
UnsupportedDatatypeError(datatype) or raise
TypeError(UNHANDLED_DATATYPE_MSG.format(type=type(datatype), value=datatype));
reference the existing raise site (the TypeError raise in this module) when
making the change.
In `@src/fastcs/transports/epics/pva/gui.py`:
- Line 58: The call that constructs a read-only wrapper AttrR(fastcs_datatype)
to feed into super()._get_write_widget is semantically wrong; instead add a
small private helper (e.g., _select_write_widget_for_datatype) that accepts the
raw DataType and encapsulates the dispatch logic, then change the call site to
call this helper with fastcs_datatype and have it delegate to
EpicsGUI._get_write_widget or the superclass logic without fabricating an AttrR;
update references to AttrR(...) in this file to use the new helper so attribute
kind is not faked.
- Around line 11-12: Imports in gui.py dig into submodules (using "from
fastcs.attributes.attr_r import AttrR" and "from fastcs.attributes.attribute
import Attribute"), which is inconsistent with package-level imports; change
them to use the package-level API by importing AttrR and Attribute from
fastcs.attributes (e.g., replace those two specific import statements with a
single "from fastcs.attributes import Attribute, AttrR"), keeping the same
symbols so other code (references to Attribute and AttrR) remains unchanged and
preserving import ordering/style used in the parent module.
In `@tests/transports/epics/ca/test_gui.py`:
- Line 96: The assertion uses Waveform(np.int32) without the explicit keyword;
update the call to use the same style as the rest of the file by changing
Waveform(np.int32) to Waveform(array_dtype=np.int32) in the assertion that calls
gui._get_write_widget(attribute=AttrR(Waveform(...))). This keeps instantiation
consistent with other lines using Waveform(array_dtype=...) and avoids
positional-argument ambiguity for the Waveform constructor.
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
Looks good except for the two comments made.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/fastcs/transports/epics/gui.py (1)
81-81: Ruff TRY003: long exception messages should live in the exception class.Both
_get_read_widget(line 81) and_get_write_widget(line 98) use the same inline-message pattern flagged by the linter. Extracting to a custom exception or shortening the inline string would silence this.♻️ One approach: a shared custom exception
+class UnsupportedDataTypeError(TypeError): + def __init__(self, datatype: object) -> None: + super().__init__(f"Unsupported type {type(datatype)}: {datatype}")Then in both match arms:
- case datatype: - raise TypeError(f"Unsupported type {type(datatype)}: {datatype}") + case datatype: + raise UnsupportedDataTypeError(datatype)Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/gui.py` at line 81, Replace the long inline TypeError messages in _get_read_widget and _get_write_widget with a short raise that uses a new custom exception class (e.g., UnsupportedDatatypeError) or a very short message; add a new UnsupportedDatatypeError exception type (subclassing TypeError) and move the detailed message construction into that class (via its __init__ or __str__) so the raise sites simply do raise UnsupportedDatatypeError(datatype) or raise UnsupportedDatatypeError; update the two raise sites (currently raising TypeError with the long f-string) to use this new exception to satisfy the linter.src/fastcs/transports/epics/pva/gui.py (2)
57-57: UseAttrWinstead ofAttrRfor semantic clarity in write-widget factory.
super()._get_write_widget(AttrR(fastcs_datatype))is functionally correct because the parent method dispatches only onattribute.datatype. However, passing a read-only attribute type to a write-widget method is semantically misleading. Since bothAttrRandAttrWaccept the samedatatypepositional argument, the refactor is safe and improves code clarity.♻️ Proposed refactor
- widget = super()._get_write_widget(AttrR(fastcs_datatype)) + widget = super()._get_write_widget(AttrW(fastcs_datatype))Update the import:
-from fastcs.attributes.attr_r import AttrR +from fastcs.attributes.attr_r import AttrR +from fastcs.attributes.attr_w import AttrW🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/pva/gui.py` at line 57, The call to super()._get_write_widget currently passes AttrR(fastcs_datatype), which is semantically misleading for a write-widget factory; change the call to use AttrW(fastcs_datatype) instead so the write-widget dispatch reflects a write attribute type, i.e. replace AttrR with AttrW where super()._get_write_widget(AttrR(fastcs_datatype)) is invoked; ensure AttrW is imported if not already.
11-12: Consider importing from the top-levelfastcs.attributespackage for consistency.The parent
gui.pyusesfrom fastcs.attributes import Attribute, AttrR, .... These submodule imports are functionally equivalent since the package re-exports both classes in__init__.py, but aligning with the convention used elsewhere keeps the import surface consistent.♻️ Proposed refactor
-from fastcs.attributes.attr_r import AttrR -from fastcs.attributes.attribute import Attribute +from fastcs.attributes import AttrR, Attribute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/pva/gui.py` around lines 11 - 12, The imports in gui.py use submodule paths (from fastcs.attributes.attr_r import AttrR and from fastcs.attributes.attribute import Attribute); update them to the top-level re-exports used elsewhere by replacing those two imports with a single import from fastcs.attributes that imports Attribute and AttrR (reference symbols: Attribute, AttrR, file's gui.py) so the import surface matches the parent module convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fastcs/transports/epics/gui.py`:
- Line 81: Replace the long inline TypeError messages in _get_read_widget and
_get_write_widget with a short raise that uses a new custom exception class
(e.g., UnsupportedDatatypeError) or a very short message; add a new
UnsupportedDatatypeError exception type (subclassing TypeError) and move the
detailed message construction into that class (via its __init__ or __str__) so
the raise sites simply do raise UnsupportedDatatypeError(datatype) or raise
UnsupportedDatatypeError; update the two raise sites (currently raising
TypeError with the long f-string) to use this new exception to satisfy the
linter.
In `@src/fastcs/transports/epics/pva/gui.py`:
- Line 57: The call to super()._get_write_widget currently passes
AttrR(fastcs_datatype), which is semantically misleading for a write-widget
factory; change the call to use AttrW(fastcs_datatype) instead so the
write-widget dispatch reflects a write attribute type, i.e. replace AttrR with
AttrW where super()._get_write_widget(AttrR(fastcs_datatype)) is invoked; ensure
AttrW is imported if not already.
- Around line 11-12: The imports in gui.py use submodule paths (from
fastcs.attributes.attr_r import AttrR and from fastcs.attributes.attribute
import Attribute); update them to the top-level re-exports used elsewhere by
replacing those two imports with a single import from fastcs.attributes that
imports Attribute and AttrR (reference symbols: Attribute, AttrR, file's gui.py)
so the import surface matches the parent module convention.
|
Thanks @jacob720 ! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/transports/epics/ca/test_gui.py (1)
96-98: Consider usingAttrWinstead ofAttrRintest_get_write_widget_none.Passing a read-only
AttrRto_get_write_widgetworks today because the parent only inspectsattribute.datatype, but a writable attribute better reflects real call-site usage and makes the test's intent clearer. If the method ever branches on access mode,AttrRwould silently test an unreachable path.🔧 Suggested change
def test_get_write_widget_none(): gui = EpicsGUI(ControllerAPI(), "DEVICE") assert ( - gui._get_write_widget(attribute=AttrR(Waveform(array_dtype=np.int32))) is None + gui._get_write_widget(attribute=AttrW(Waveform(array_dtype=np.int32))) is None )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transports/epics/ca/test_gui.py` around lines 96 - 98, Test uses a read-only AttrR when calling gui._get_write_widget in test_get_write_widget_none; change the test to pass a writable attribute (use AttrW with the same Waveform(array_dtype=np.int32) argument) so the test reflects real call-site usage and avoids exercising a read-only path that may become irrelevant if access-mode logic is added; update the call in the test (the gui._get_write_widget(attribute=...)) to use AttrW instead of AttrR and ensure any necessary imports or symbols remain available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/transports/epics/ca/test_gui.py`:
- Around line 96-98: Test uses a read-only AttrR when calling
gui._get_write_widget in test_get_write_widget_none; change the test to pass a
writable attribute (use AttrW with the same Waveform(array_dtype=np.int32)
argument) so the test reflects real call-site usage and avoids exercising a
read-only path that may become irrelevant if access-mode logic is added; update
the call in the test (the gui._get_write_widget(attribute=...)) to use AttrW
instead of AttrR and ensure any necessary imports or symbols remain available.
2 participants
Fixes #298
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Breaking Changes