Add additional pattern matches for error recognition #1240
Conversation
|
Warning Rate limit exceeded@mulkieran has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds explicit type hints to functions in src/stratis_cli/_error_reporting.py; centralizes error-interpretation orchestration; caches D-Bus name lookups; and adds branches for missing GetManagedObjects properties and property-retrieval failures, with functions returning Optional[str]. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
mulkieran
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/stratis_cli/_error_reporting.py (2)
185-203: KeyError-based message is helpful; consider guarding against emptyKeyError.argsThe new branch that inspects a secondary
KeyErrorand emits a property-specific message is a nice improvement in diagnostics, especially the inclusion of the interface and property name.One small robustness tweak you might consider: if
KeyErrorwere ever raised without arguments,maybe_key_error.args[0]would itself raise anIndexError. A defensive pattern would be something like:- if isinstance(maybe_key_error, KeyError): - return ( - f'Property "{maybe_key_error.args[0]}" belonging to ' + if isinstance(maybe_key_error, KeyError): + prop_name = maybe_key_error.args[0] if maybe_key_error.args else "<?>" + return ( + f'Property "{prop_name}" belonging to ' f'interface "{error.interface_name}" is not in this ' "GetManagedObjects result. This could be due to the fact " "that stratisd omitted the property from the " "GetManagedObjects result because there was an error in " "obtaining that value." )Not strictly necessary if you fully control the
KeyErrorconstruction, but it hardens this path against unexpected call sites.
263-335: New GetProperty-context handling underDBus.Error.Failedis consistent and preciseThe addition of the pylint
too-many-return-statementssuppression on_interpret_errors_2is appropriate given the pattern-matching style here.The new
DPClientGetPropertyContextbranch under theorg.freedesktop.DBus.Error.Failedcase mirrors the existing SetProperty handling and gives a clear, property-specific message including the daemon’s error text. This fits well with the existing interpretations forGetManagedObjectsandSetPropertyand doesn’t alter behavior for other contexts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.12)
- GitHub Check: test-runs (3.13)
🔇 Additional comments (1)
src/stratis_cli/_error_reporting.py (1)
115-155: Localdbus_namereuse keeps logic unchanged and improves clarityUsing a single
dbus_name = error.get_dbus_name()and reusing it for theAccessDeniedandNameHasNoOwner/ServiceUnknownchecks is a clean refactor: behavior is preserved while avoiding repeated calls and keeping the conditionals easier to read.
mulkieran
force-pushed
the
recognize-failed-dbus-name
branch
from
f6bc3ca to
e58bfa6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stratis_cli/_actions/_physical.py (1)
134-141: Critical: Table header/data column count mismatch.The table data now has 6 columns (including the new InitializationTime at lines 134-136), but
print_tableis called with only 5 headers at line 141. This mismatch will cause display issues or runtime errors.Apply this diff to add the missing header:
print_table( - ["Pool Name", "Device Node", "Physical Size", "Tier", "UUID"], + ["Pool Name", "Device Node", "Physical Size", "Tier", "UUID", "Initialization Time"], sorted(tables, key=lambda entry: (entry[0], entry[1])), - ["<", "<", ">", ">", "<"], + ["<", "<", ">", ">", "<", "<"], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/stratis_cli/_actions/_data.py(1 hunks)src/stratis_cli/_actions/_physical.py(2 hunks)src/stratis_cli/_error_reporting.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stratis_cli/_error_reporting.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/stratis_cli/_actions/_physical.py (1)
src/stratis_cli/_actions/_connection.py (1)
get_object(44-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
- GitHub Check: check-typos
🔇 Additional comments (2)
src/stratis_cli/_actions/_data.py (1)
88-91: LGTM! Good consistency improvement.Adding the
Devclass alongsideMODevbrings the blockdev interface in line with the existing patterns for filesystem and pool interfaces, where both a direct class and a managed object class are generated.src/stratis_cli/_actions/_physical.py (1)
49-49: Verify that Dev import is intentionally part of this PR.The PR title indicates this is about "error recognition," but the changes add device InitializationTime display functionality. Confirm this feature addition is intentional for this PR.
| Dev.Properties.InitializationTime.Get( | ||
| get_object("/org/storage/stratis3/1") | ||
| ), |
There was a problem hiding this comment.
Critical: Hardcoded object path yields incorrect per-device values.
The same hardcoded path "/org/storage/stratis3/1" is used for all devices in the loop. This means every device row will display the same InitializationTime value (or an error if that path is invalid), rather than each device's individual InitializationTime.
To retrieve the correct InitializationTime for each device, you need to:
- Capture the device object path in the
modevslist comprehension (lines 58-73) - Use that object path when calling
Dev.Properties.InitializationTime.Get()
Apply this diff to capture device object paths:
modevs = [
- MODev(info)
+ (objpath, MODev(info))
for objpath, info in devs(
props=(
None
if pool_name is None
else {
"Pool": next(
pools(props={"Name": pool_name})
.require_unique_match(True)
.search(managed_objects)
)[0]
}
)
).search(managed_objects)
]Then update the table building to use per-device paths:
tables = [
[
- path_to_name.get(modev.Pool(), TABLE_UNKNOWN_STRING),
- paths(modev),
- size(modev),
- tier_str(modev.Tier()),
- format_uuid(modev.Uuid()),
+ path_to_name.get(modev[1].Pool(), TABLE_UNKNOWN_STRING),
+ paths(modev[1]),
+ size(modev[1]),
+ tier_str(modev[1].Tier()),
+ format_uuid(modev[1].Uuid()),
Dev.Properties.InitializationTime.Get(
- get_object("/org/storage/stratis3/1")
+ get_object(modev[0])
),
]
- for modev in modevs
+ for modev in modevs
]Alternatively, unpack the tuple in the comprehension for cleaner code:
tables = [
[
path_to_name.get(modev.Pool(), TABLE_UNKNOWN_STRING),
paths(modev),
size(modev),
tier_str(modev.Tier()),
format_uuid(modev.Uuid()),
Dev.Properties.InitializationTime.Get(
- get_object("/org/storage/stratis3/1")
+ get_object(objpath)
),
]
- for modev in modevs
+ for objpath, modev in modevs
]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/stratis_cli/_actions/_physical.py around lines 58-73 and 134-136, the
code uses a hardcoded object path "/org/storage/stratis3/1" when fetching
InitializationTime for every device; capture each device's actual object path in
the modevs list comprehension (include the object path in the tuple you build
per device or make the comprehension produce (device, object_path) pairs), then
update the table-building code to call
Dev.Properties.InitializationTime.Get(object_path) for that device instead of
the hardcoded string; alternatively unpack the tuple from the comprehension into
(dev, path) for clearer usage when building rows.
1e048ac to
719fd24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/stratis_cli/_error_reporting.py (3)
163-205: New handling forDbusClientMissing*with underlyingKeyErroris good; consider tightening the property name formattingThe added logic that inspects
errors[1]for aKeyErrorand surfaces a message about a missing property in theGetManagedObjectsresult is a useful improvement and matches the expected chain shape (ActionError -> DbusClientMissing* -> KeyError -> ...).One small polish: using
maybe_key_error.argsin the f‑string will render a tuple (e.g."('PropName',)") for the common single‑argument case. You might want to prefer the first argument when present for a cleaner message, e.g.:- if len(errors) > 1: - maybe_key_error = errors[1] - if isinstance(maybe_key_error, KeyError): - return ( - f'Property "{maybe_key_error.args}" belonging to ' + if len(errors) > 1: + maybe_key_error = errors[1] + if isinstance(maybe_key_error, KeyError) and maybe_key_error.args: + return ( + f'Property "{maybe_key_error.args[0]}" belonging to ' f'interface "{error.interface_name}" is not in this ' "GetManagedObjects result. This could be due to the fact " "that stratisd omitted the property from the " "GetManagedObjects result because there was an error in " "obtaining that value." )This keeps the new behaviour but improves readability of the diagnostic.
362-385:_interpret_errorsorchestration is consistent, but empty subchains rely on the broadexceptDelegating directly to
_interpret_errors_1(errors[1:])after asserting aStratisCliActionErrorhead matches the docstring (“subchain after the first error”) and works for the usual multi‑element chains.If an action error somehow has no cause (i.e.
errorslength is 1),_interpret_errors_1will see an empty list and raiseIndexError, which is then swallowed by the broadexceptand results inNone. That’s safe but a bit indirect; you could optionally guard against this and avoid the extra exception:- try: - assert isinstance(errors[0], StratisCliActionError) - - return _interpret_errors_1(errors[1:]) + try: + assert isinstance(errors[0], StratisCliActionError) + if len(errors) > 1: + return _interpret_errors_1(errors[1:]) + return NoneThis preserves behaviour but removes reliance on the catch‑all for a predictable edge case.
387-412: Return type ofhandle_errordoes not match its behaviour
handle_erroris annotated as-> Optional[str], but all paths either raise (raise err) or callexit_, and the function never returns the explanation string to the caller. At runtime this is fine, but the annotation is misleading and may confuse type checkers or readers.Consider one of:
- If callers never consume a return value (current behaviour), change the signature to reflect non‑returning semantics, e.g.
-> Noneor-> NoReturn(withfrom typing import NoReturn).- If you intend to surface the explanation for programmatic use, explicitly
return explanationinstead of callingexit_directly, and move the exit/printing logic to the caller.Aligning the annotation with the actual control flow will make the intent clearer and keep static analysis happy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
🔇 Additional comments (3)
src/stratis_cli/_error_reporting.py (3)
21-72: Type imports and_interface_name_to_common_nameannotations look consistentImporting
List/Optionaland addinginterface_name: str -> strare consistent with current usage and docstrings; no behaviour change or issues here.
101-160: Refactor in_interpret_errors_0to cachedbus_nameis safeUsing a local
dbus_name = error.get_dbus_name()and reusing it in the subsequent checks keeps semantics identical while avoiding repeated calls. The existing AccessDenied and NameHasNoOwner/ServiceUnknown handling remains correct.
264-337: Extended_interpret_errors_2forDPClientGetPropertyContextis coherent with existing patternsThe new branch that detects
DPClientGetPropertyContextwhen the D‑Bus error isorg.freedesktop.DBus.Error.Failedmirrors the existingDPClientSetPropertyContexthandling and provides a clear, context‑rich message for get‑property failures. Interface and property name usage look correct, and the control flow still falls back to other handlers when no specific context matches.
mulkieran
force-pushed
the
recognize-failed-dbus-name
branch
from
719fd24 to
674ede4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/stratis_cli/_error_reporting.py (1)
163-204: Tighter handling of missing property errors; consider refining KeyError formattingThe new branch for
DbusClientMissingSearchPropertiesError/DbusClientMissingPropertyErrorwith a followingKeyErrorgives a much more actionable explanation, which is helpful.One minor improvement: in Line 197,
maybe_key_error.argsis usually a tuple, so the message will show something like("prop_name",)rather than justprop_name. Using the first arg orstr(maybe_key_error)would read more naturally.For example:
- if isinstance(maybe_key_error, KeyError): - return ( - f'Property "{maybe_key_error.args}" belonging to ' + if isinstance(maybe_key_error, KeyError): + prop_name = maybe_key_error.args[0] if maybe_key_error.args else maybe_key_error + return ( + f'Property "{prop_name}" belonging to ' f'interface "{error.interface_name}" is not in this ' "GetManagedObjects result. This could be due to the fact " "that stratisd omitted the property from the " "GetManagedObjects result because there was an error in " "obtaining that value." )This keeps the new behavior but produces a cleaner user-facing message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/stratis_cli/_error_reporting.py (1)
src/stratis_cli/_errors.py (1)
StratisCliActionError(360-382)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.12)
- GitHub Check: test-runs (3.13)
🔇 Additional comments (6)
src/stratis_cli/_error_reporting.py (6)
21-21: Typing imports are consistent with added annotationsImporting
ListandOptionalmatches the new function signatures and keeps typing centralized here. No issues.
64-87: Type annotation on_interface_name_to_common_nameis appropriateThe
interface_name: str -> strsignature matches how this helper is used and should improve type checking without altering behavior.
101-160: Refactor of_interpret_errors_0to usedbus_nameandOptional[str]looks goodCaching
error.get_dbus_name()intodbus_namereduces repetition and makes the subsequent comparisons clearer, and theOptional[str]return type matches the documented “may return None” behavior. No behavioral regressions apparent.
264-337: New DPClientGetPropertyContext handling is coherent with existing patternsExtending
_interpret_errors_2to recognizeDPClientGetPropertyContextunderorg.freedesktop.DBus.Error.Failedmirrors the existingDPClientSetPropertyContexthandling and yields a clear, targeted explanation about failing to get a property. The use ofcontext.property_nameanderror.interface_namealigns with the other branches.
362-377: Type annotation and explicit length assertion in_interpret_errorsare reasonableAnnotating
_interpret_errorsas takingList[BaseException]and returningOptional[str]matches its actual usage. Theassert len(errors) > 1makes the expectation that there is at least one underlying cause explicit; any violation is still safely absorbed by the surroundingtry/exceptand results in the generic “unexpected error” path, as before.
388-413: Narrowinghandle_error’s parameter type clarifies intentTyping
errasStratisCliActionErrordocuments the precondition already described in the comments and aligns with howget_errors(err)is consumed. The function’s behavior—either raising/printing a generic message or exiting with a specific explanation—remains unchanged.
mulkieran
force-pushed
the
recognize-failed-dbus-name
branch
from
674ede4 to
a1e1fe0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/stratis_cli/_error_reporting.py (1)
391-416: Optional: Consider adding return type annotation.The parameter type hint is appropriate. For completeness, you could add
-> NoReturnas the return type annotation (requiresfrom typing import NoReturn), since the function either callsexit_()or raises an exception. However, this is purely optional—the function's behavior is clear from its implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/stratis_cli/_error_reporting.py (1)
src/stratis_cli/_errors.py (1)
StratisCliActionError(360-382)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.12)
- GitHub Check: test-runs (3.13)
🔇 Additional comments (6)
src/stratis_cli/_error_reporting.py (6)
21-21: LGTM: Type imports added for function annotations.The addition of
ListandOptionalfrom the typing module supports the new type hints throughout the file, improving code clarity and type safety.
64-64: LGTM: Type annotations added.The type hints correctly document that the function accepts and returns a string.
101-122: LGTM: Good refactor to cache D-Bus name.The introduction of the
dbus_namevariable eliminates redundant calls toerror.get_dbus_name(), improving efficiency. The type hints accurately reflect the function signature.
186-207: LGTM: Comprehensive handling for missing GetManagedObjects properties.The new branch effectively handles the case where stratisd omits a property from the GetManagedObjects result due to an error. The logic:
- Safely checks for a chained KeyError
- Extracts the property key with appropriate fallback
- Provides a detailed, actionable error message
The implementation correctly handles edge cases and falls back to the generic message when the specific pattern doesn't match.
332-340: LGTM: Added handling for D-Bus property retrieval failures.The new branch provides detailed error reporting when getting a D-Bus property fails, maintaining consistency with the existing
SetPropertyContexthandling. The error message appropriately includes the property name, interface name, and the underlying D-Bus error.
365-381: LGTM: Strengthened precondition check.The additional assertion ensures that the error chain contains at least one error beyond the initial
StratisCliActionError. This prevents potential issues when calling_interpret_errors_1(errors[1:])and properly handles cases where no interpretable error chain exists.
mulkieran
force-pushed
the
recognize-failed-dbus-name
branch
3 times, most recently
from
6ecf3e8 to
7f100ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/stratis_cli/_error_reporting.py (1)
21-21: Type hints look consistent; consider minor annotation/docstring refinementsThe added
List/Optionalimports and annotations on_interface_name_to_common_name,_interpret_errors_0/1/2, and_interpret_errorsall line up with how these functions are used and what they return. Two small, optional polish items:
handle_errornever returns normally (it always raises), so annotating it asNoReturncan help static analysis.- Docstrings for
_interpret_errors_1/_interpret_errors_2/_interpret_errorsstill mention “list of Exception” / “None or str”; if you care about strict consistency with type hints, you could update those to matchList[BaseException]/Optional[str].Example diff if you choose to adopt this:
-from typing import List, Optional +from typing import List, Optional, NoReturn @@ -def _interpret_errors(errors: List[BaseException]) -> Optional[str]: +def _interpret_errors(errors: List[BaseException]) -> Optional[str]: @@ -def handle_error(err: StratisCliActionError): +def handle_error(err: StratisCliActionError) -> NoReturn: @@ - :param errors: the chain of errors - :type errors: list of Exception - :returns: None if no interpretation found, otherwise str + :param errors: the chain of errors + :type errors: list of BaseException + :returns: None if no interpretation found, otherwise str(and similarly for the other two functions’ docstrings, if desired).
Also applies to: 64-64, 101-103, 163-165, 267-269, 365-365, 391-391
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/stratis_cli/_error_reporting.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/stratis_cli/_error_reporting.py (1)
src/stratis_cli/_errors.py (1)
StratisCliActionError(360-382)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-runs (3.13)
- GitHub Check: test-runs (3.12)
🔇 Additional comments (4)
src/stratis_cli/_error_reporting.py (4)
101-103: Cachingdbus_nameimproves clarity without changing behaviorUsing a local
dbus_name = error.get_dbus_name()and then comparing against that value in theAccessDeniedand name/owner cases is a clean, behavior‑preserving refactor. It avoids repeated method calls and makes the matching logic easier to follow. No issues spotted here.Also applies to: 116-137
186-188: New KeyError branch gives a precise GetManagedObjects diagnosticThe additional handling for
DbusClientMissingSearchPropertiesError/DbusClientMissingPropertyErrorwhen followed by aKeyErrorin the chain looks solid:
- You guard on
len(errors) > 1before accessingerrors[1].- You confirm the second error is a
KeyErrorand safely accessargs[0]only if present.- When the pattern does not match, you still fall back to
_DBUS_INTERFACE_MSG, so you preserve existing behavior.This should yield much more actionable messages for missing properties in
GetManagedObjectswithout introducing new failure modes.Also applies to: 193-207
267-269: GetProperty context handling is consistent with existing SetProperty branchThe new
DPClientGetPropertyContextbranch under theDBus.Error.Failedcase mirrors the existingDPClientSetPropertyContextmessaging:
- It keys off the same
next_error.get_dbus_name() == "org.freedesktop.DBus.Error.Failed"guard.- It uses
context.property_nameanderror.interface_nameto build a clear, user‑facing explanation.- It reuses
next_error.get_dbus_message()to surface daemon‑provided detail.The control flow remains safe: if the context is not a
DPClientGetPropertyContext, this branch is skipped and other handling applies as before. No correctness concerns.Also applies to: 332-340
365-382: Stricter precondition on error chains avoids index errors and centralizes behaviorTightening
_interpret_errorswithassert len(errors) > 1before delegating to_interpret_errors_1(errors[1:])is a good defensive step:
- It guarantees
_interpret_errors_1always receives a non‑empty list, preventing anIndexErroronerrors[0].- Violations drop into the broad
exceptand result inNone, which in turn driveshandle_errordown the generic “unexpected error” path — a sensible fallback.
handle_error’s orchestration ofget_errors(err),_interpret_errors, and the generic fallback (printing a helpful message and re‑raising when no explanation is found) remains coherent and unchanged in spirit; only the internal precondition and typing were tightened. This looks correct.Also applies to: 391-416
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
1 participant
No description provided.