Search handler groups #71
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications across four files within the flogin package. An import statement is added in Changes
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
flogin/search_group.py (2)
33-39: Add a class-level docstring for clarity.
Although the class is well-structured and typed, consider adding a top-level docstring to describe its purpose, usage, and main responsibilities. This will improve readability and maintainability.
200-238: Check for potential conflicts when creating subgroups.
The subgroup method allows prefix nesting. Consider validating that a newly created subgroup prefix does not collide with existing handlers or subgroups in self._subhandlers. This helps avoid accidental shadowing or confusing behavior.flogin/__init__.py (1)
11-11: Avoid wildcard imports to prevent namespace clutter.
Importing using '*' can inadvertently pollute the namespace. Explicit imports are generally recommended to maintain clarity and avoid name collisions.flogin/utils.py (1)
301-309: Enhance documentation for the new 'owner' parameter.
The addition of the keyword-only parameter owner improves clarity around instance ownership. Consider extending or adding docstrings to explain acceptable types, usage patterns, and implications if the owner is missing or not set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flogin/__init__.py(1 hunks)flogin/search_group.py(1 hunks)flogin/utils.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
flogin/search_group.py
[low] 102-102: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
[low] 103-103: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
[low] 108-108: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
🔇 Additional comments (2)
flogin/search_group.py (2)
40-48: Consider adding tests for prefix initialization errors.
The conditional logic raising a ValueError is appropriate. However, adding unit tests that verify behavior for missing prefix arguments ensures stronger coverage of edge cases and clarifies contract expectations.
66-71: Validate query.text before calling startswith.
Although the code handles IndexError, an empty or None query.text could still pose edge cases. Consider validating query.text isn't empty or None before using string operations to avoid hidden exceptions or subtle bugs.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
flogin/search_group.py (1)
159-161: Consider adding error handling for empty subgroups.The
root_handlermethod assumes subgroups exist but doesn't handle the case when_subgroupsis empty.Consider adding a check:
async def root_handler(self, query: Query) -> list[Result]: + if not self._subgroups: + return [] return [self.create_result(key, query) for key in self._subgroups]flogin/__init__.py (1)
11-11: Consider updating__all__to expose new SearchGroup class.While the wildcard import from search_group is added, consider explicitly listing SearchGroup in
__all__for better control over the public API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/api.rst(1 hunks)flogin/__init__.py(1 hunks)flogin/search_group.py(1 hunks)flogin/utils.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
flogin/search_group.py (1)
Learnt from: cibere
PR: cibere/flogin#71
File: flogin/search_group.py:102-108
Timestamp: 2025-02-06T22:49:41.181Z
Learning: In flogin, assert statements in callback functions (e.g., `assert self.plugin`, `assert self.plugin.last_query`) are used for Pyright type narrowing and not for runtime validation, as these values are guaranteed to be present when flogin calls the callbacks.
🪛 GitHub Actions: build
flogin/search_group.py
[error] 5-5: ImportError: cannot import name 'PluginT' from 'flogin._types' (unknown location). Did you mean: 'plugin'?
🪛 GitHub Actions: Lint
flogin/search_group.py
[low] 142-142: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
[low] 143-143: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
[low] 148-148: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
🔇 Additional comments (4)
flogin/search_group.py (2)
23-78: Well-documented class with clear examples!The docstring provides comprehensive examples of using the
SearchGroupclass for creating nested command structures. The examples demonstrate both basic subcommand usage and nested subgroup creation.
142-143: Assert statements are correctly used for type narrowing.Based on the retrieved learning from @cibere, these assert statements are intentionally used for Pyright type narrowing and not for runtime validation, as these values are guaranteed to be present when flogin calls the callbacks.
Also applies to: 148-148
🧰 Tools
🪛 GitHub Actions: Lint
[low] 142-142: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
[low] 143-143: Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
flogin/utils.py (1)
301-309: Good improvement to owner initialization!The changes make the owner initialization more flexible by allowing it to be set during instantiation while maintaining backward compatibility with the MISSING sentinel.
docs/api.rst (1)
106-111: Documentation properly structured and placed!The SearchGroup documentation is correctly placed in the Search Handlers section and properly uses attributetable and autoclass directives.
| parts = query.text.split(self.sep) | ||
| query._data["search"] = query.text.removeprefix(self.prefix).removeprefix( | ||
| self.sep | ||
| ) | ||
|
|
||
| try: | ||
| handler = self._subgroups[parts[1]] | ||
| except (KeyError, IndexError): | ||
| handler = self.root_handler | ||
|
|
||
| if isinstance(handler, SearchGroup): | ||
| handler.plugin = self.plugin | ||
| handler = handler.callback | ||
|
|
||
| return handler(query) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid query format.
The callback method splits the query text but doesn't validate the format. This could lead to unexpected behavior with malformed queries.
Consider adding validation:
def callback(self, query: Query) -> SearchHandlerCallbackReturns:
+ if not query.text:
+ return self.root_handler(query)
+
parts = query.text.split(self.sep)
query._data["search"] = query.text.removeprefix(self.prefix).removeprefix(
self.sep
)
try:
handler = self._subgroups[parts[1]]
except (KeyError, IndexError):
handler = self.root_handler
if isinstance(handler, SearchGroup):
handler.plugin = self.plugin
handler = handler.callback
return handler(query)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts = query.text.split(self.sep) | |
| query._data["search"] = query.text.removeprefix(self.prefix).removeprefix( | |
| self.sep | |
| ) | |
| try: | |
| handler = self._subgroups[parts[1]] | |
| except (KeyError, IndexError): | |
| handler = self.root_handler | |
| if isinstance(handler, SearchGroup): | |
| handler.plugin = self.plugin | |
| handler = handler.callback | |
| return handler(query) | |
| def callback(self, query: Query) -> SearchHandlerCallbackReturns: | |
| if not query.text: | |
| return self.root_handler(query) | |
| parts = query.text.split(self.sep) | |
| query._data["search"] = query.text.removeprefix(self.prefix).removeprefix(self.sep) | |
| try: | |
| handler = self._subgroups[parts[1]] | |
| except (KeyError, IndexError): | |
| handler = self.root_handler | |
| if isinstance(handler, SearchGroup): | |
| handler.plugin = self.plugin | |
| handler = handler.callback | |
| return handler(query) |
There was a problem hiding this comment.
can you explain this more? I don't understand why this change would help
There was a problem hiding this comment.
@cibere Let me explain why this change would be helpful:
Currently, when an empty query is received, the code still tries to:
- Split the empty string (
"") into parts - Remove prefixes from an empty string
- Try to access
parts[1]which fails becausepartsis just[""] - Finally falls back to
root_handlerdue to the IndexError
The suggested change simply checks for empty query upfront and returns root_handler immediately. While both approaches end up calling root_handler, the suggested way:
- Is more explicit about handling empty queries
- Avoids unnecessary string operations
- Makes the code's intent clearer
Would you like me to show some example scenarios to demonstrate this?
There was a problem hiding this comment.
@cibere Let me demonstrate with two scenarios:
Scenario 1: Current Behavior (Empty Query)
query.text = ""
# Current flow:
parts = "".split(" ") # parts = [""]
query._data["search"] = "".removeprefix(self.prefix).removeprefix(self.sep) # Unnecessary operations
try:
handler = self._subgroups[parts[1]] # IndexError because parts only has one item
except (KeyError, IndexError):
handler = self.root_handler # Finally reaches root_handlerScenario 2: Proposed Behavior (Empty Query)
query.text = ""
# Proposed flow:
if not query.text:
return self.root_handler(query) # Immediately returns root_handlerThe end result is the same (calling root_handler), but the proposed way:
- Skips unnecessary string operations
- Avoids throwing and catching exceptions
- Makes it immediately clear that empty queries show the root menu
Would this help explain the reasoning better?
| from ._types import ( | ||
| PluginT, | ||
| SearchHandlerCallback, | ||
| SearchHandlerCallbackReturns, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix import error for PluginT.
The pipeline is failing with ImportError: cannot import name 'PluginT' from 'flogin._types'. This needs to be fixed for the code to work.
🏁 Script executed:
#!/bin/bash
# Check if PluginT is defined in _types.py
rg -A 5 'PluginT' | grep -v 'search_group.py'Length of output: 17092
I'll take a closer look at the contents of flogin/_types.py to understand the conditional definition of PluginT and why it might not be available when imported by search_group.py. Let me inspect the file.
🏁 Script executed:
#!/bin/bash
# Output the first 100 lines of flogin/_types.py to inspect definitions for PluginT.
sed -n '1,100p' flogin/_types.pyLength of output: 773
Investigated conditional definition of PluginT in flogin/_types.py.
The file clearly defines PluginT in both branches of the TYPE_CHECKING condition—when type checking (using the advanced definition with bounds and covariance) and at runtime (a plain TypeVar). This indicates that PluginT is indeed available from flogin/_types.py. The pipeline’s ImportError appears to be caused by a circular dependency issue during type checking—specifically, when TYPE_CHECKING is True, _types.py attempts to import Plugin from flogin/plugin.py, and a circular import may be preventing PluginT from being exported properly.
To resolve this, please consider one of the following fixes:
-
Move the definition of PluginT outside of the TYPE_CHECKING block so that it’s unconditionally available. For example, always define PluginT with the advanced TypeVar using importlib “if TYPE_CHECKING:” only for imports needed solely for type annotations. This avoids a circular dependency when the type checker is active.
-
Alternatively, if you need to keep the conditional definition, restructure your code (and possibly the import order in flogin/plugin.py) to break the circular dependency.
Please verify which approach best fits the overall design of our codebase.
🧰 Tools
🪛 GitHub Actions: build
[error] 5-5: ImportError: cannot import name 'PluginT' from 'flogin._types' (unknown location). Did you mean: 'plugin'?
1 participant
Summary
Adds search groups to easily create a nested command structure
Changelog
Breaking Changes
New Features
Bug Fixes
Checklist
Summary by CodeRabbit
SearchGroupclass for structured command group creation.SearchGroupclass, including its attributes and members.