feat(dotfile): add support for ignoring sections in collected files#103
feat(dotfile): add support for ignoring sections in collected files#103
Conversation
Add optional parameter to CollectFromPaths to skip content between SHELLTIME IGNORE BEGIN and SHELLTIME IGNORE END markers. This allows users to exclude sensitive information from dotfile collection. Fixes #102 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @AnnatarHe's task —— View job Code ReviewI've completed a thorough review of this pull request. Here's my detailed feedback:
✅ Positive Aspects
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @AnnatarHe, 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 introduces a significant enhancement to how dotfiles are collected, allowing users to explicitly mark and ignore sensitive sections within their configuration files. By adding a skipIgnoredSections parameter to the CollectFromPaths function and implementing a robust filtering mechanism, the system can now prevent confidential data from being inadvertently included. This improves security and flexibility for managing dotfiles, with all existing dotfile applications updated to leverage this new capability by default.
Highlights
- New Ignore Sections Feature: An optional boolean parameter
skipIgnoredSectionshas been added to theCollectFromPathsfunction, allowing content between specific ignore markers to be skipped during collection. - Content Filtering Logic: A new
filterIgnoredSectionsfunction has been implemented to parse file content and remove lines found betweenSHELLTIME IGNORE BEGINandSHELLTIME IGNORE ENDmarkers. - Default Behavior Update: All existing dotfile application implementations have been updated to utilize the new
skipIgnoredSectionsparameter, with its default value set totrueto enable content filtering by default. - Comprehensive Testing: New and updated tests have been added to ensure the ignore functionality works correctly, covering scenarios where sections are both skipped and included.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| 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 pull request 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 introduces a valuable feature for ignoring specific sections within dotfiles. The implementation is generally solid and includes good test coverage. My review provides suggestions to enhance code clarity, reduce duplication, and improve the robustness of the tests. The most significant recommendation is to simplify how CollectFromPaths is called across all application-specific implementations, which will remove considerable boilerplate code. Additionally, there are opportunities to refactor duplicated logic within CollectFromPaths and refine the filterIgnoredSections function for better maintainability.
| skipIgnored := true | ||
| return b.CollectFromPaths(ctx, b.Name(), b.GetConfigPaths(), &skipIgnored) |
There was a problem hiding this comment.
This implementation can be simplified. The CollectFromPaths function is designed to handle a nil value for skipIgnoredSections by defaulting to true. Therefore, you can just pass nil directly.
This simplification applies to all other CollectDotfiles methods in the other dotfile_*.go files as well, which would remove a lot of boilerplate code.
| skipIgnored := true | |
| return b.CollectFromPaths(ctx, b.Name(), b.GetConfigPaths(), &skipIgnored) | |
| return b.CollectFromPaths(ctx, b.Name(), b.GetConfigPaths(), nil) |
| func (b *BaseApp) filterIgnoredSections(content string) string { | ||
| lines := strings.Split(content, "\n") | ||
| var filteredLines []string | ||
| var inIgnoreBlock bool | ||
|
|
||
| for _, line := range lines { | ||
| // Check for ignore markers (can be in comments) | ||
| if strings.Contains(line, "SHELLTIME IGNORE BEGIN") { | ||
| inIgnoreBlock = true | ||
| continue | ||
| } | ||
| if strings.Contains(line, "SHELLTIME IGNORE END") { | ||
| inIgnoreBlock = false | ||
| continue | ||
| } | ||
|
|
||
| // Only include lines that are not in an ignore block | ||
| if !inIgnoreBlock { | ||
| filteredLines = append(filteredLines, line) | ||
| } | ||
| } | ||
|
|
||
| return strings.Join(filteredLines, "\n") | ||
| } |
There was a problem hiding this comment.
The implementation of filterIgnoredSections can be improved for readability and efficiency:
- Use constants for markers: The strings
"SHELLTIME IGNORE BEGIN"and"SHELLTIME IGNORE END"are used as magic strings. Defining them as constants at the package or function level would improve readability and make them easier to manage. - Pre-allocate slice capacity: You can pre-allocate the capacity for
filteredLinesto avoid multiple re-allocations as the slice grows. This is a good practice for performance, especially with larger files.
func (b *BaseApp) filterIgnoredSections(content string) string {
const ignoreBeginMarker = "SHELLTIME IGNORE BEGIN"
const ignoreEndMarker = "SHELLTIME IGNORE END"
lines := strings.Split(content, "\n")
filteredLines := make([]string, 0, len(lines))
var inIgnoreBlock bool
for _, line := range lines {
// Check for ignore markers (can be in comments)
if strings.Contains(line, ignoreBeginMarker) {
inIgnoreBlock = true
continue
}
if strings.Contains(line, ignoreEndMarker) {
inIgnoreBlock = false
continue
}
// Only include lines that are not in an ignore block
if !inIgnoreBlock {
filteredLines = append(filteredLines, line)
}
}
return strings.Join(filteredLines, "\n")
}|
|
||
| t.Run("collect from single file", func(t *testing.T) { | ||
| dotfiles, err := app.CollectFromPaths(ctx, "testapp", []string{configFile}) | ||
| skipIgnored := true |
There was a problem hiding this comment.
The variable skipIgnored is repeatedly declared with := true inside each t.Run block (e.g., here, and on lines 152, 174, 182). To reduce repetition, you can declare it once before all the sub-tests.
| assert.NotContains(t, dotfiles[0].Content, "secret_key") | ||
| assert.NotContains(t, dotfiles[0].Content, "password=hidden") | ||
| assert.NotContains(t, dotfiles[0].Content, "SHELLTIME IGNORE") | ||
| assert.Contains(t, dotfiles[0].Content, "line1") | ||
| assert.Contains(t, dotfiles[0].Content, "line2") | ||
| assert.Contains(t, dotfiles[0].Content, "visible_key=value") |
There was a problem hiding this comment.
The assertions here check for the presence or absence of certain substrings. This is good, but it can be made more robust by asserting the exact expected content. This ensures that no extra lines or characters are present and that the line order is correct.
For the skipIgnored = true case, you could assert:
expectedContent := "line1\nline2\nvisible_key=value"
assert.Equal(t, expectedContent, dotfiles[0].Content)Similarly, for the skipIgnored = false case, you can assert that the content is identical to the original configContentWithIgnore.
1 participant
Summary
CollectFromPathsto skip content between ignore markersfilterIgnoredSectionsfunction to remove lines betweenSHELLTIME IGNORE BEGINandSHELLTIME IGNORE ENDtrueChanges
BaseApp.CollectFromPathsto accept optionalskipIgnoredSectionsparameter (defaults totrue)filterIgnoredSectionsmethod to filter out sensitive contenttruefor the new parameterTest Plan
skipIgnored=trueandskipIgnored=falseFixes #102
🤖 Generated with Claude Code