feat: add filter parameter to file_browser for flexible filtering#8751
feat: add filter parameter to file_browser for flexible filtering#8751by22Jy wants to merge 2 commits intomarimo-team:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
Adds a new backend-side filter parameter to mo.ui.file_browser() to support flexible file inclusion rules (regex or callback) beyond the existing filetypes extension filtering.
Changes:
- Introduces a
filterparameter tofile_browser.__init__(regex string viare.matchorCallable[[Path], bool]). - Adds
_should_include_file()and updates_list_directory()to use unified filtering logic. - Expands the
file_browserdocstring with usage examples for regex and callback filtering.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _should_include_file(self, file: Path, is_directory: bool) -> bool: | ||
| """Determine if a file should be included based on filter/filetypes. | ||
|
|
||
| Args: | ||
| file: The file path to check | ||
| is_directory: Whether the path is a directory | ||
|
|
||
| Returns: | ||
| bool: True if the file should be included, False otherwise | ||
| """ | ||
| # Priority: filter > filetypes | ||
| if self._filter is not None: | ||
| # Apply custom filter | ||
| if isinstance(self._filter, str): | ||
| # Regex filter (match against file.name) | ||
| try: | ||
| return bool(re.match(self._filter, file.name)) | ||
| except re.error: | ||
| # Invalid regex pattern, treat as no match | ||
| LOGGER.warning( | ||
| f"Invalid regex pattern in filter: {self._filter}" | ||
| ) | ||
| return False | ||
| else: | ||
| # Callback filter | ||
| try: | ||
| return self._filter(file) | ||
| except Exception as e: | ||
| # Callback raised exception, log and exclude file | ||
| LOGGER.warning( | ||
| f"Filter callback raised exception for {file}: {e}" | ||
| ) | ||
| return False | ||
|
|
||
| # Fall back to filetypes | ||
| if self._filetypes and not is_directory: | ||
| return file.suffix.lower() in self._filetypes | ||
|
|
||
| return True |
There was a problem hiding this comment.
New filter behavior is not covered by the existing tests/_plugins/ui/_impl/test_file_browser.py suite (which currently focuses on filetypes and ignore_empty_dirs). Adding tests for regex filters (valid/invalid), callback filters (normal/exception), and filter precedence over filetypes would help prevent regressions—especially around directory visibility/navigation when filter is set.
| # Priority: filter > filetypes | ||
| if self._filter is not None: | ||
| # Apply custom filter | ||
| if isinstance(self._filter, str): | ||
| # Regex filter (match against file.name) | ||
| try: | ||
| return bool(re.match(self._filter, file.name)) | ||
| except re.error: |
There was a problem hiding this comment.
When filter is provided, _should_include_file applies it to directories too (no is_directory guard). This can hide folders and prevent users from navigating into them, unlike the existing filetypes behavior which only filters non-directories. Consider always including directories (or only applying filter when not is_directory, unless selection_mode intentionally wants directory-name filtering).
| try: | ||
| return bool(re.match(self._filter, file.name)) | ||
| except re.error: | ||
| # Invalid regex pattern, treat as no match | ||
| LOGGER.warning( | ||
| f"Invalid regex pattern in filter: {self._filter}" | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Regex filtering currently calls re.match(self._filter, file.name) for every entry, and an invalid pattern will raise/log on every file, potentially spamming logs and adding avoidable overhead. Consider compiling/validating the regex once in __init__ (store a compiled re.Pattern or disable filtering after one warning) and then using pattern.match(...) during listing.
| try: | |
| return bool(re.match(self._filter, file.name)) | |
| except re.error: | |
| # Invalid regex pattern, treat as no match | |
| LOGGER.warning( | |
| f"Invalid regex pattern in filter: {self._filter}" | |
| ) | |
| return False | |
| # Compile the pattern once and cache it; if invalid, log once. | |
| invalid = getattr(self, "_filter_invalid", False) | |
| if invalid: | |
| # Previously determined to be invalid; treat as no match. | |
| return False | |
| pattern = getattr(self, "_compiled_filter", None) | |
| if pattern is None: | |
| try: | |
| pattern = re.compile(self._filter) | |
| except re.error: | |
| # Invalid regex pattern, treat as no match and log once. | |
| LOGGER.warning( | |
| f"Invalid regex pattern in filter: {self._filter}" | |
| ) | |
| setattr(self, "_filter_invalid", True) | |
| return False | |
| else: | |
| setattr(self, "_compiled_filter", pattern) | |
| return bool(pattern.match(file.name)) |
| else: | ||
| # Callback filter | ||
| try: | ||
| return self._filter(file) | ||
| except Exception as e: | ||
| # Callback raised exception, log and exclude file | ||
| LOGGER.warning( | ||
| f"Filter callback raised exception for {file}: {e}" | ||
| ) | ||
| return False |
There was a problem hiding this comment.
If filter is neither a str nor a callable, the code falls into the callback branch and will raise TypeError at call time, which gets caught and logged as a warning and silently excludes the file. Since filter is part of the public API, it would be better to validate the type in __init__ and raise a ValueError for unsupported values so misconfiguration is detected early.
|
@by22Jy Were you still interested in contributing this? Happy to give a deeper look once you sign the CLA |
Summary
Adds a
filterparameter tomo.ui.file_browser()for more flexible file filtering beyond simple file extensions.Fixes #8399
Changes
filterparameter tofile_browser.__init__()that accepts:file.nameusingre.match)Pathand returnsbool_should_include_file()method to handle filtering logic_list_directory()to use the new unified filtering methodimport reat the top of the fileImplementation Details
Filter Priority:
filter>filetypesWhen
filteris provided, it takes precedence overfiletypes. IffilterisNone, the existingfiletypesbehavior is used (backward compatible).Error Handling:
Backward Compatibility: ✅
filetypesparameter retainedfilter=None)Examples
Regex Filter
Callback Filter
Testing
This PR includes the core functionality. Tests should cover:
Notes
_has_files_recursive) still usesfiletypesonly, not thefilterparameter (for simplicity and consistency)re.match()for regex matching (notre.search()), matching against the full filenameRelated
enhancement,api-change,good first issue