feat: refine import parsing logic to avoid false positives - #1
Open
ksprashu wants to merge 6 commits into
Open
Conversation
The previous import parsing logic was too broad and would incorrectly identify strings like npm packages, python decorators, and email addresses as import paths. This change refines the logic to be more specific, requiring import paths to start with `./`, `../`, `/`, or not contain a `/` at all. This change also adds tests to verify that the new logic correctly ignores these false positives. Fixes google-gemini#2967
There was a problem hiding this comment.
Summary of Changes
Hello @ksprashu, 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 refines the import parsing logic within the "memoryImportProcessor" to prevent false positives. The changes ensure that strings resembling imports, such as npm package names in code blocks, Python decorators, or email addresses, are no longer incorrectly identified as import paths, thereby improving the accuracy and robustness of the import processing mechanism.
Highlights
- Refined Import Parsing Logic: The "findImports" function in "memoryImportProcessor.ts" has been updated with more precise validation rules for identifying import paths.
- Prevention of False Positives: The new logic specifically prevents misidentification of patterns like npm-style package names, Python decorators, and email addresses as valid import paths.
- Enhanced Test Coverage: New test cases have been added to "memoryImportProcessor.test.ts" to explicitly verify that these non-import patterns are correctly ignored by the import processor.
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.
Code Review
This pull request refines the import parsing logic to prevent false positives from strings like npm package names, Python decorators, or email addresses. The changes to the validation logic in memoryImportProcessor.ts are effective at this, and are supported by new tests. However, I've identified a critical regression in the new logic that prevents valid imports from subdirectories (e.g., @foo/bar.md) from being recognized. I've provided a suggestion to fix this while maintaining the benefit of avoiding false positives.
Comment on lines
+124
to
+127
| (importPath.startsWith('./') || | ||
| importPath.startsWith('../') || | ||
| importPath.startsWith('/') || | ||
| !importPath.includes('/')) |
There was a problem hiding this comment.
The new validation logic is a great improvement for avoiding false positives. However, it introduces a regression where valid imports for files in subdirectories without an explicit ./ prefix are no longer recognized. For example, an import like @components/button.md would be ignored because its path contains a / but doesn't start with ./, ../, or /.
To fix this while still avoiding false positives like npm packages, we can refine the logic to also allow paths that contain a slash if they appear to have a file extension (i.e., contain a . in their final path segment).
I'd also recommend adding a new test case to cover this scenario, for example with an import like @foo/bar.md.
(importPath.startsWith('./') ||
importPath.startsWith('../') ||
importPath.startsWith('/') ||
!importPath.includes('/') ||
importPath.split('/').pop()!.includes('.'))
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This commit addresses a regression where valid imports for files in subdirectories without an explicit ./ prefix were not recognized (e.g., @components/button.md). The logic is refined to allow paths that contain a slash if they also include a file extension in their final path segment. This fixes the regression while still avoiding false positives from things like npm package scopes. This change is based on a code review suggestion from Gemini Code Assist.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This change prevents decorators (e.g., `@my-decorator`) from being incorrectly identified as file imports during memory processing. - Updated the import detection logic to correctly differentiate between decorators and valid import paths. - Added a new test case to ensure that decorators are ignored and do not trigger file read operations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes google-gemini#2967