Skip to content
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

fix: Adding a new preprocess step to the include option #103

Merged

Conversation

Lucas-1048
Copy link
Contributor

@Lucas-1048 Lucas-1048 commented Mar 22, 2025

The preprocessing step works as follows: if a curly brace is found in the path, the function expand_brace_patterns() will be called on the patterns and will add the base path to the patterns within {}.

This resolves the issue where the first item inside {} was not considered. Furthermore, it ensures that only files and directories within the base path are included (before this change, the entire project references were being considered).

@Lucas-1048 Lucas-1048 force-pushed the fix/glob-pattern-tool-curly-brackets branch 2 times, most recently from 31d3ad4 to abdfaa4 Compare March 23, 2025 04:33
@Lucas-1048 Lucas-1048 force-pushed the fix/glob-pattern-tool-curly-brackets branch from abdfaa4 to 5a1a9ce Compare March 24, 2025 22:29
@ODAncona
Copy link
Collaborator

Hello @Lucas-1048,
Thank you for your contribution.
I wrote test and pushed them to main branch to check the correctness of your patch.

    fn test_brace_expansion_first_item() {
        let base_path = TEST_DIR.path();

        // This pattern uses brace expansion to match foo.py, bar.py, and baz.py
        // The issue was that the first item (foo.py) wasn't being considered
        let include_patterns = build_globset(&vec!["lowercase/{foo.py,bar.py,baz.py}".to_string()]);
        let exclude_patterns =
            build_globset(&vec!["lowercase/{qux.py,corge.py,grault.py}".to_string()]);
        let include_priority = false;

        // ALL files in the brace expansion should be included
        for file in ["foo.py", "bar.py", "baz.py"] {
            let path = base_path.join("lowercase").join(file);
            let relative_path = path.strip_prefix(base_path).unwrap();

            assert!(
                should_include_file(
                    &relative_path,
                    &include_patterns,
                    &exclude_patterns,
                    include_priority
                ),
                "Failed to include file: {}",
                file
            );
        }

        // Files not in the brace expansion should be excluded
        for file in ["qux.txt", "corge.txt", "grault.txt"] {
            let path = base_path.join("lowercase").join(file);
            let relative_path = path.strip_prefix(base_path).unwrap();

            assert!(
                !should_include_file(
                    &relative_path,
                    &include_patterns,
                    &exclude_patterns,
                    include_priority
                ),
                "Incorrectly included non-matching file: {}",
                file
            );
        }
    }
    ```
Congratulations 🥳 All tests pass. 

However, as I reviewed your PR, we handle the logic for an edge case that should be directed treated in the `globset` library. It doesn't make sense to fix it in `code2prompt` if the problem should be fixed upstream. Because fixing in `globset` directly would benefit all dependent projects and prevent the need for similar workarounds elsewhere ...

I will merge your code now anyway as a temporary fix. However, it shouldn't be a permanent solution. Could you directly investigate in the upstream `ripgrep` repo ?

Next steps:
 -  I redacted [an issue in the official repo](https://github.com/BurntSushi/ripgrep/issues/3018)
 - You should take a look at [Actual globset file](https://github.com/BurntSushi/ripgrep/blob/master/crates/globset/src/glob.rs)
 
 Thank you,
 
 Let me know what do you think about this ?
 
 Olivier

@ODAncona ODAncona linked an issue Mar 25, 2025 that may be closed by this pull request
@ODAncona ODAncona self-requested a review March 25, 2025 03:36
@ODAncona ODAncona merged commit 7bd92bd into mufeedvh:main Mar 25, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glob pattern tool curly brackets
2 participants