-
Notifications
You must be signed in to change notification settings - Fork 9.5k
feat(core): share file list patterns between glob and grep tools #6359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): share file list patterns between glob and grep tools #6359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @Han5991, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors how file exclusion patterns are managed across the codebase. It centralizes previously disparate and hardcoded exclusion lists into a new ignorePatterns.ts module, introducing a FileExclusions class to provide consistent and configurable patterns to various tools. This change eliminates duplication, improves the reliability of file processing by ensuring all tools respect the same exclusion rules, and sets the stage for future command-line configurability of these patterns. The update also includes enhancements to binary file detection and fixes for related tests.
Highlights
- Consolidated File Exclusion Patterns: File exclusion patterns, previously duplicated and inconsistent across tools like
glob,grep, andread-many-files, have been centralized into a newignorePatterns.tsfile. - Introduction of
FileExclusionsClass: A newFileExclusionsclass has been introduced to manage and provide consistent sets of ignore patterns for different use cases, eliminating code duplication and improving maintainability. - Integration of Shared Patterns: Hardcoded exclusion lists in
glob,grep, andread-many-fileshave been replaced with calls to theFileExclusionsclass, ensuring all tools use the same, up-to-date patterns. - Enhanced Binary File Detection: A
BINARY_EXTENSIONSconstant has been added for efficient and consistent detection of binary files, improving file type identification. - Future Configurability: The
Configinterface now includes a placeholdergetCustomExcludes()method, laying the groundwork for future CLI configurability of exclusion patterns. - Test Suite Enhancements: Tests in
packages/cli/src/config/config.test.tswere updated to ensure consistent locale setup and enhanced extension paths, fixing previously affected tests.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of refactoring and centralizing file exclusion patterns into a new FileExclusions class, which improves maintainability and consistency across different tools. The changes are well-structured and the new tests are comprehensive.
I've found one high-severity issue related to the construction of the BINARY_EXTENSIONS constant, which appears to be a regression from the previous implementation. My review includes a specific suggestion to address this.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of centralizing file exclusion patterns into a new FileExclusions class, which significantly improves maintainability and consistency across the glob, grep, and read-many-files tools. The new implementation successfully removes duplicated code and lays a solid foundation for future configuration. I've identified a couple of minor areas where the consolidation could be even more complete, and a potential omission based on the PR description. My feedback focuses on these points to fully realize the goal of this refactoring.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of centralizing file exclusion patterns into a new FileExclusions class, which significantly improves maintainability and consistency across tools. However, I've identified a critical issue that breaks PDF file processing due to incorrect exclusion, and a high-severity issue in the grep tool that unnecessarily restricts directory exclusion patterns. Addressing these points will ensure the refactoring is both robust and functionally correct.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great refactoring that centralizes file exclusion patterns into a new ignorePatterns.ts utility. This significantly improves maintainability and consistency across the glob, grep, and read-many-files tools by removing duplicated, hardcoded lists. The introduction of the FileExclusions class is well-designed, and the new unit tests for it are comprehensive.
My review includes one high-severity comment regarding a potential issue in the extractExtensionsFromPatterns helper function, which could fail to parse glob patterns with brace expansion correctly. Addressing this will make the new utility more robust for future changes.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of centralizing file exclusion patterns into a new FileExclusions class, which significantly improves maintainability and consistency across different tools. My feedback focuses on ensuring this new logic is applied consistently across all relevant tools and fixing a bug in a new helper function to make the implementation more robust.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a solid refactoring that successfully centralizes file exclusion logic into a new FileExclusions class. This significantly reduces code duplication across the glob, grep, and read-many-files tools, improving maintainability and consistency. The introduction of shared constants for ignore patterns and a comprehensive test suite for the new logic are excellent additions.
I've identified a couple of high-severity issues related to the robustness of pattern handling in edge cases. My review includes specific suggestions to address these points. Once these are resolved, this will be a very strong contribution to the codebase.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a well-executed refactoring that centralizes file exclusion patterns into a new FileExclusions utility. This significantly improves maintainability and consistency across the glob, grep, and read-many-files tools by removing duplicated, hardcoded ignore lists. The introduction of BINARY_EXTENSIONS also cleans up binary file detection. The changes are supported by a comprehensive new test suite for the new ignorePatterns module, which is excellent.
I've found one high-severity issue in the grep tool where the logic to determine which patterns should be passed to grep --exclude-dir could lead to incorrect behavior. My feedback includes a specific suggestion to make this logic more robust.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of centralizing file exclusion patterns into a new FileExclusions class, which significantly improves maintainability and consistency across the glob, grep, and read-many-files tools. The changes are well-structured and accompanied by a comprehensive new test suite. I've found one high-severity issue in the grep tool where the logic for converting glob patterns to --exclude-dir arguments is too restrictive, which could lead to inconsistent behavior. Addressing this will make the implementation more robust.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of refactoring file exclusion patterns into a centralized FileExclusions utility, which significantly improves maintainability and consistency across the glob, grep, and read-many-files tools. The new module is well-structured and the changes are applied correctly throughout the codebase. I've found one high-severity issue in the grep tool where the logic to parse these new exclusion patterns for grep --exclude-dir is too restrictive and could miss valid directory exclusion patterns. A fix is suggested to make this logic more robust.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of centralizing the file exclusion patterns into a new FileExclusions class. This significantly improves maintainability and consistency across the glob, grep, and read-many-files tools. The new implementation is well-structured and the addition of comprehensive tests is excellent. I have one suggestion to improve the logic for extracting directory patterns for the native grep tool to better support custom exclusion patterns with wildcards.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great improvement, centralizing file exclusion patterns into a new ignorePatterns.ts utility. This significantly reduces code duplication and improves maintainability across the glob, grep, and read-many-files tools. The new FileExclusions class is well-designed and the accompanying tests are comprehensive.
I've found one high-severity issue in the grep tool implementation related to inconsistent behavior when using the system grep strategy. My feedback includes a detailed explanation and a code suggestion to resolve it. Other than that, the changes look excellent.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively centralizes file exclusion patterns into a new FileExclusions class, which is a great improvement for maintainability and consistency across tools like glob, grep, and read-many-files. The changes are well-structured, and the addition of a comprehensive test suite for the new ignorePatterns.ts utility is commendable. However, I've found a critical issue in the grep tool's logic that could lead to incorrect behavior when handling custom file-based exclusion patterns.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great refactoring that centralizes file exclusion patterns into a new FileExclusions class, significantly improving maintainability and consistency across tools like glob, grep, and read-many-files. The changes are well-structured and include comprehensive tests for the new logic. My review focuses on ensuring the refactoring is complete and robust. I've identified a couple of areas for improvement: one to make the pattern extraction more robust against edge cases, and another to fully centralize all common version control ignore patterns for consistency.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively centralizes file exclusion patterns into a new FileExclusions class, which is a great step towards improving maintainability and consistency across different tools. The refactoring in glob, read-many-files, and related utilities correctly adopts this new shared logic. However, I've identified a high-severity issue in the grep tool's implementation. The logic for extracting directory names for exclusion is overly restrictive, which could lead to grep searching inside directories that should have been ignored. My review includes a specific suggestion to fix this.
|
/gemini summary |
- Add `**/bower_components/**` to default ignore patterns. - Extract `MEDIA_FILE_PATTERNS` for specialized media file handling. - Update tests to reflect the added ignore pattern. - Adjust grep tool to streamline common exclude directories.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Removed `.pdf` from ignore patterns to streamline exclusions. - Simplified `COMMON_IGNORE_PATTERNS` processing logic in `grep.ts`.
…e patterns - Introduced `extractExtensionsFromPatterns` for robust extension extraction supporting brace expansions. - Extended `BINARY_EXTENSIONS` with additional media file types (e.g., `.pdf`, `.jpg`, `.png`). - Added comprehensive tests for edge cases in `extractExtensionsFromPatterns`.
…ility - Refactored `grep.ts` to use `FileExclusions` for centralized pattern handling. - Updated `ignorePatterns.ts` to fix substring extraction logic for better accuracy. - Enhanced exclusion logic with `getGlobExcludes()` for dynamic ignore patterns.
…add tests - Updated `ignorePatterns.ts` to enhance extension extraction with `path.extname`, supporting compound extensions, dotfiles, and edge cases. - Added comprehensive test cases to validate new behaviors in `ignorePatterns.test.ts`. - Refined grep exclusion logic to filter glob patterns excluding file-type-specific matches.
- Adjusted `filter` logic to include patterns ending with `/**`. - Removed obsolete JSDoc parameter description in `getDescription()`.
- Refined `globExcludes` processing to handle patterns ending with `/**` and `/` more effectively. - Improved filtering logic to exclude patterns with wildcards.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
… handling - Updated `dir` logic to exclude patterns starting with `*.`. - Simplified filtering logic by removing wildcard checks in `.filter()`.
- Adjusted `dir` handling to improve detection of likely directories. - Excluded patterns with wildcards and clarified filtering for dotnames.
- Added `.svn` and `.hg` to default ignore patterns in `ignorePatterns.ts`. - Updated test cases in `ignorePatterns.test.ts` to validate new patterns. - Improved directory filtering in `grep.ts` by enhancing dot-pattern exclusion logic.
- Consolidated condition checks for likely directory patterns. - Removed redundant filtering logic for dotnames.
- Added `getFileExclusions` method to `Config` for managing file exclusion logic. - Updated tools (`grep`, `glob`, `read-many-files`) to utilize `Config.getFileExclusions` for exclusion patterns. - Modified tests to mock `getFileExclusions` behavior and validate proper exclusion handling. - Exported ignore patterns (`COMMON_IGNORE_PATTERNS`, `DEFAULT_FILE_EXCLUDES`) for consistency across modules.
5555fa1 to
2c4d1a0
Compare
…glob and grep tools (google-gemini#6359)
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Arya Gummadi <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Arya Gummadi <[email protected]>
…gle-gemini#6359) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Arya Gummadi <[email protected]>
…gle-gemini#6359) Co-authored-by: Arya Gummadi <[email protected]>
ignorePatterns.ts.glob,grep, andread-many-filestools with shared constants.BINARY_EXTENSIONSfor efficient binary file detection.loadCliConfigwith consistent locale setup and enhanced extension paths.TLDR
This PR addresses the TODO comment in read-many-files.ts by creating a centralized FileExclusions class that provides consistent file exclusion patterns across tools. The implementation eliminates code duplication, improves maintainability, and sets the foundation for future CLI configurability.
Dive Deeper
Problem
Previously, glob.ts and read-many-files.ts maintained separate hardcoded exclusion lists:
This led to inconsistent behavior and maintenance overhead.
Solution
Created a FileExclusions class that:
Key Changes
Reviewer Test Plan
This test suite comprehensively validates:
What was fixed
Linked issues / bugs