Conversation
|
Hi @mihirkhandekar , for your review when you have time. 😄 |
There was a problem hiding this comment.
Pull request overview
This PR adds wildcard-based filtering functionality to the hook feature, allowing users to suppress notifications for specific commands. The implementation includes new CLI commands for managing filters and integrates filter checking into the existing shell hooks for both Bash and Zsh.
Changes:
- Added hook filter management methods to
MessagingConfigclass (add, remove, list, clear) - Implemented new CLI commands
hook-filterandhook-check-filterto manage and check filters - Updated shell hook scripts to check filters before sending notifications
- Version bumped from 0.2.7 to 0.2.8 across all project files
- Documentation updated in README files (English, Spanish, Chinese, Hindi) and CHANGELOG
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| telert/messaging.py | Added four new methods to MessagingConfig for managing hook filters stored in config.json |
| telert/cli.py | Added do_hook_filter() and do_hook_check_filter() functions; updated hook script generation to integrate filter checking; added argument parsers for new commands |
| telert/init.py | Version bump to 0.2.8 |
| pyproject.toml | Version bump to 0.2.8 |
| vscode-telert/package.json | Version bump to 0.2.8 |
| packaging/docker/server.py | Version bump to 0.2.8 |
| README.md | Added documentation section for hook filtering with examples |
| README.zh-CN.md | Added Chinese translation of hook filtering documentation |
| README.hi.md | Added Hindi translation of hook filtering documentation |
| README.es.md | Added Spanish translation of hook filtering documentation |
| CHANGELOG.md | Added entry for version 0.2.8 with feature description |
| .gitignore | Added CLAUDE.md to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def do_hook_check_filter(a): | ||
| """Check if a command matches any hook filter (for shell hook use).""" | ||
| import fnmatch |
There was a problem hiding this comment.
The fnmatch module is imported inside the function rather than at the module level. While this works correctly, it's inconsistent with the rest of the codebase where all imports are at the top of the file. For consistency and following Python conventions (PEP 8), consider moving the import to the top of the file with other standard library imports.
There was a problem hiding this comment.
Fixed — moved import fnmatch to the top of the file with the other standard library imports. See commit de90133.
| for pattern in filters: | ||
| if fnmatch.fnmatch(command, pattern): | ||
| # Match found - exit with 0 (filtered) | ||
| sys.exit(0) | ||
|
|
||
| # No match - exit with 1 (not filtered) | ||
| sys.exit(1) |
There was a problem hiding this comment.
The exit code logic appears to be inverted relative to standard Unix conventions and the shell hook expectations. The function exits with 0 (success) when a filter matches and 1 when no filter matches. However, in the shell hooks, the command is inverted with '!' (e.g., 'if ! telert hook-check-filter'), which means the hook expects exit code 0 to mean "not filtered" and non-zero to mean "filtered". This double inversion works but is confusing and non-standard. Consider changing the exit codes so that 0 means "not filtered" (allow notification) and 1 means "filtered" (suppress notification), and remove the '!' negation from the shell hooks. This would make the behavior more intuitive and easier to maintain.
There was a problem hiding this comment.
Fixed — swapped the exit codes so that 0 means "not filtered" (allow notification) and 1 means "filtered" (suppress notification), following standard Unix conventions. Removed the ! negation from all 3 shell hook locations (2 zsh, 1 bash). See commit de90133.
| def add_hook_filter(self, pattern: str) -> bool: | ||
| """Add a hook message filter pattern. | ||
|
|
||
| Args: | ||
| pattern: Wildcard pattern to filter messages (e.g., "cd *", "ls*") | ||
|
|
||
| Returns: | ||
| True if added, False if already exists | ||
| """ | ||
| filters = self.get_hook_filters() | ||
| if pattern in filters: | ||
| return False | ||
| filters.append(pattern) | ||
| self._config["hook_filters"] = filters | ||
| self.save() | ||
| return True |
There was a problem hiding this comment.
The pattern parameter is not validated for empty strings or whitespace-only strings before being added to filters. Adding an empty or whitespace pattern would match every command (since fnmatch treats empty pattern specially), effectively disabling all notifications. Consider adding validation to reject empty or whitespace-only patterns in the add_hook_filter method.
There was a problem hiding this comment.
Fixed — added validation to reject empty or whitespace-only patterns in add_hook_filter() before they can be added to the filter list. See commit de90133.
…nd validation - Move fnmatch import to module level per PEP 8 - Fix exit code convention: 0=not filtered, 1=filtered (standard Unix) - Remove shell hook ! negation to match new exit codes - Validate empty/whitespace patterns in add_hook_filter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@mihirkhandekar, I addressed the feedback, please review. |
Added feature to use wildcard filters in order to filter out hook messages.
By default, I want to know when any long-running command finishes. However, there are some that I would like to exclude from the alerts as they are not important enough to be notified. In order to reduce the noise, I (w/ Claude) implemented a filter that applies to hook functionality only.
New functionality allows adding, removing, listing and clearing the hook filters.
I've selected wildcards over regex for ease of use, assuming that most use cases are straight forward.
Feedback is welcome.