chore: add depguard and forbidigo lint rules for FileIO adoption#342
chore: add depguard and forbidigo lint rules for FileIO adoption#342
Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
size/M
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions that don't block merge. The change is linting config only with no runtime code. The depguard rule is correctly scoped to shortcuts/, vfs.ReadDir() is verified to exist so the consolidation is accurate, and CI passes. The only open finding is a P2 gap in the filepath.Glob migration guidance. No files require special attention.
|
| Filename | Overview |
|---|---|
| .golangci.yml | Adds depguard rule scoped to shortcuts/, consolidates forbidigo regex patterns, and extends filepath interception; one gap: filepath.Glob is blocked but vfs.Glob() does not yet exist, leaving internal/ developers without a concrete migration path |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["New / Modified Code"] --> B{Layer?}
B -->|"shortcuts/**"| C["forbidigo check\nos.*, filepath.*"]
B -->|"shortcuts/**"| D["depguard check\nno internal/vfs imports"]
B -->|"internal/**\n(except vfs/)"| E["forbidigo check\nos.*, filepath.*"]
B -->|"Other layers\n(cmd/, etc.)"| F["forbidigo: excluded\ndepguard: not scoped here"]
B -->|"_test.go (any)"| G["Both linters: excluded"]
C --> H{"Violation?"}
D --> H
E --> H
H -->|os.Stat/Open/…| I["→ vfs.Xxx()"]
H -->|os.Create/Temp| J["→ vfs.CreateTemp() or streaming"]
H -->|os.Mkdir| K["→ vfs.MkdirAll()"]
H -->|filepath.Walk/Glob/…| L["→ vfs helpers / runtime.FileIO()"]
H -->|"import internal/vfs\n(shortcuts only)"| M["→ runtime.FileIO()"]
Reviews (2): Last reviewed commit: "chore: add depguard and forbidigo rules ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 92-98: Remove the duplicate os\.Exit pattern (one of the two
identical rules for os\.Exit\b) and resolve the shadowing between the broad
os\.Std(in|out|err)\b rule and the specific os\.Stdin\b / os\.Stdout\b /
os\.Stderr\b rules: either delete the broad os\.Std(in|out|err)\b entry so the
specific messages for os.Stdin/os.Stdout/os.Stderr (e.g., the
IOStreams.In/Out/ErrOut guidance) remain effective, or delete the specific
os\.Stdin/os\.Stdout/os\.Stderr entries if you prefer the single generic
message—do not keep both.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b534fb0714617832e61c86c0d1fe5aae15ad8bf2🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-lint-depguard -y -g |
- Add depguard linter to block shortcuts/ from importing internal/vfs directly (must use runtime.FileIO() instead) - Add forbidigo rules for os.* filesystem ops, IO streams, os.Exit, and filepath.* functions that bypass vfs - Split os.Remove / os.RemoveAll into separate patterns with accurate guidance (RemoveAll not yet in vfs) - Use compact regex groups for maintainability, no duplicate or shadowed patterns Change-Id: I9e45ab07ca58a61b86bdcea9f1f2cc6181c974bc
tuxedomm
force-pushed
the
feat/fileio-lint-depguard
branch
from
7cfb237 to
b534fb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.golangci.yml (1)
66-72: MentionResolveSavePath()for write-path migrations.
shortcuts/common/runner.go:318-331documentsResolveSavePath()as the shortcut helper for output paths, whileValidatePath()is for input-path validation. The current lint text only points toValidatePath()/FileIO(), so save-path migrations will get incomplete guidance.Also applies to: 103-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 66 - 72, Update the lint rule descriptions to mention ResolveSavePath() as the helper for write/save path migrations in addition to calling out runtime.ValidatePath() for input-path validation and runtime.FileIO() for file operations; specifically, replace or augment references to ValidatePath()/FileIO() in the existing messages (the ones that currently forbid importing internal/vfs or internal/vfs/localfileio) to include ResolveSavePath() so authors get correct guidance for output paths when migrating shortcuts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 75-94: The rules for os.* patterns conflict with the new depguard
rule forbidding internal/vfs imports from shortcuts/, so update .golangci.yml to
remove the conflicting remediation: either split the existing
os\.(Stat|Lstat|Open|OpenFile|Rename|ReadFile|WriteFile|Getwd|UserHomeDir|ReadDir)\b
and related patterns into two path-scoped rules (one targeting internal/ with
messages pointing to internal/vfs and one targeting shortcuts/ with messages
pointing to runtime.FileIO()/runtime.ValidatePath()), or change the messages for
all shortcuts/ entries (e.g., the messages attached to patterns like
os\.(Create|CreateTemp|MkdirTemp)\b, os\.Mkdir(All)?\b, os\.Remove\b,
os\.RemoveAll\b,
os\.(Chdir|Chmod|Chown|Lchown|Chtimes|CopyFS|DirFS|Link|Symlink|Readlink|Truncate|SameFile)\b)
to reference runtime.FileIO()/runtime.ValidatePath() instead of internal/vfs so
the linter guidance no longer conflicts with depguard.
---
Nitpick comments:
In @.golangci.yml:
- Around line 66-72: Update the lint rule descriptions to mention
ResolveSavePath() as the helper for write/save path migrations in addition to
calling out runtime.ValidatePath() for input-path validation and
runtime.FileIO() for file operations; specifically, replace or augment
references to ValidatePath()/FileIO() in the existing messages (the ones that
currently forbid importing internal/vfs or internal/vfs/localfileio) to include
ResolveSavePath() so authors get correct guidance for output paths when
migrating shortcuts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
2 participants
Summary
depguardlinter to blockshortcuts/from importinginternal/vfsdirectly — new code must useruntime.FileIO()forbidigomessages to provide layered guidance:internal/→ use vfs,shortcuts/→ use FileIOfilepath.(EvalSymlinks|Walk|WalkDir|Glob|Abs)interceptionCI uses
--new-from-rev=origin/mainso only new/modified code is checked — no impact on existing files.Test plan
golangci-lint run --new-from-rev=origin/main ./shortcuts/...— 0 issuesSummary by CodeRabbit