[CDF-25167] 🚫 Toolkit ignore #1718
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
|
/gemini review |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces .toolkitignore functionality, allowing users to specify files and directories to be excluded from the cdf build command. The implementation adds new classes for pattern parsing and integrates this logic into the ModuleLocation data class. While the overall approach is sound, a critical issue exists in how ignore patterns are applied, which could lead to incorrect filtering of files and directories. Additionally, there are minor maintainability improvements that can be made.
| if not ignore_parser.is_ignored(Path(directory_name), is_directory=True): | ||
| invalid_resource_directory.add(directory_name) |
There was a problem hiding this comment.
The ignore_parser.is_ignored method is currently called with Path(directory_name), which represents only the top-level directory name relative to the module. This is incorrect because .toolkitignore patterns can target specific files or nested directories (e.g., sub_dir/file.txt or nested/folder/). For accurate pattern matching, the is_ignored method should be called with the full relative_to_module path, which provides the complete path relative to the module's root. This is a critical correctness issue as it prevents proper filtering of ignored files and directories, leading to unexpected build outcomes.
| if not ignore_parser.is_ignored(Path(directory_name), is_directory=True): | |
| invalid_resource_directory.add(directory_name) | |
| if not ignore_parser.is_ignored(relative_to_module, is_directory=True): | |
| invalid_resource_directory.add(directory_name) |
| if not is_file_in_resource_folder: | ||
| invalid_resource_directory.add(relative_to_module.parts[0]) | ||
| directory_name = relative_to_module.parts[0] | ||
| directory_path = self.dir / directory_name |
There was a problem hiding this comment.
The variable directory_path is defined but not used. Unused variables add clutter and can be misleading for future maintainers. It's good practice to remove them to improve code readability and maintainability1.
Style Guide References
| directory_path = self.dir / directory_name | |
| # directory_path = self.dir / directory_name |
Footnotes
| """ | ||
|
|
||
| import fnmatch | ||
| import re |
There was a problem hiding this comment.
Description
Changelog
cdf
Added
.toolkitignoreto tell Toolkit what to ignore in thecdf buildcommand.templates
No changes.