Conversation
UnstoppableMango
commented
Mar 21, 2026
- Vibe
- The old tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
=======================================
Coverage 83.23% 83.23%
=======================================
Files 26 26
Lines 680 680
=======================================
Hits 566 566
Misses 106 106
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors the github submodule to delegate GitHub-backed filesystem behavior to the external github.com/unstoppablemango/ihfs/ghfs implementation, removing the in-repo GitHub path parsing and per-resource filesystem packages (user/repository/release/content/asset) and consolidating tests at the module level.
Changes:
- Replaced the previous
githubfilesystem stack (ghpath,user,repository,release,content,asset) with a thin wrapper aroundihfs/ghfs. - Added an
afero.Fileadapter (github.File) that wrapsio/fsfiles returned byghfs. - Consolidated/updated tests and updated
github/go.mod+github/go.sumdependencies accordingly.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| github/fs.go | Switches Fs to wrap *ghfs.Fs and adapts Open/Stat to the new backend. |
| github/file.go | Adds an afero.File adapter over io/fs.File with optional interface support. |
| github/github_suite_test.go | Moves shared test client init into the module-level suite. |
| github/fs_test.go | New consolidated filesystem tests targeting ghfs-style paths. |
| github/file_test.go | Updates file tests to use the new module-level Fs and paths; adds archive-dir test. |
| github/go.mod | Removes ghpath-related deps and adds github.com/unstoppablemango/ihfs/ghfs. |
| github/go.sum | Updates checksums to reflect removed/added dependencies. |
| github/user/user_suite_test.go | Removed (tests consolidated at module level). |
| github/user/fs.go | Removed (superseded by ghfs). |
| github/user/fs_test.go | Removed (tests consolidated at module level). |
| github/user/fileinfo.go | Removed (superseded by ghfs). |
| github/user/file.go | Removed (superseded by ghfs). |
| github/repository/repository_suite_test.go | Removed (tests consolidated at module level). |
| github/repository/fs.go | Removed (superseded by ghfs). |
| github/repository/fs_test.go | Removed (tests consolidated at module level). |
| github/repository/fileinfo.go | Removed (superseded by ghfs). |
| github/repository/file.go | Removed (superseded by ghfs). |
| github/repository/file_test.go | Removed (tests consolidated at module level). |
| github/repository/release/release_suite_test.go | Removed (tests consolidated at module level). |
| github/repository/release/fs.go | Removed (superseded by ghfs). |
| github/repository/release/fs_test.go | Removed (tests consolidated at module level). |
| github/repository/release/fileinfo.go | Removed (superseded by ghfs). |
| github/repository/release/file.go | Removed (superseded by ghfs). |
| github/repository/release/file_test.go | Removed (tests consolidated at module level). |
| github/repository/release/asset/asset_suite_test.go | Removed (tests consolidated at module level). |
| github/repository/release/asset/fs.go | Removed (superseded by ghfs). |
| github/repository/release/asset/fs_test.go | Removed (tests consolidated at module level). |
| github/repository/release/asset/fileinfo.go | Removed (superseded by ghfs). |
| github/repository/release/asset/file.go | Removed (superseded by ghfs). |
| github/repository/release/asset/file_test.go | Removed (tests consolidated at module level). |
| github/repository/content/content_suite_test.go | Removed (tests consolidated at module level). |
| github/repository/content/fs.go | Removed (superseded by ghfs). |
| github/repository/content/fs_test.go | Removed (tests consolidated at module level). |
| github/repository/content/fileinfo.go | Removed (superseded by ghfs). |
| github/repository/content/file.go | Removed (superseded by ghfs). |
| github/repository/content/directoryinfo.go | Removed (superseded by ghfs). |
| github/repository/content/directory.go | Removed (superseded by ghfs). |
| github/ghpath/path.go | Removed (path parsing delegated to ghfs). |
| github/ghpath/path_test.go | Removed (path parsing delegated to ghfs). |
| github/ghpath/ghpath_suite_test.go | Removed (tests consolidated at module level). |
| github/internal/context.go | Removed (no longer needed after refactor). |
| github/internal/constraint.go | Removed (no longer needed after refactor). |
Comments suppressed due to low confidence (1)
github/file_test.go:22
- These tests exercise
ghx.NewFs(client)with a realinternal.DefaultClient()and then callOpen/Readdirnames, which will hit the live GitHub API but are not labeledE2E. To keepmake test(which excludesE2E) reliable and offline-friendly, consider marking the network-dependent specs asE2Eor using a mock HTTP transport/server.
| It("should open user", func() { | ||
| fs := ghx.NewFs(client) | ||
|
|
||
| file, err := fs.Open("users/UnstoppableMango") | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(file.Name()).To(Equal("users/UnstoppableMango")) | ||
| }) | ||
|
|
||
| It("should open user file", func() { | ||
| fs := ghx.NewFs(client) | ||
|
|
||
| file, err := fs.OpenFile("users/UnstoppableMango", 69, os.ModePerm) | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(file.Name()).To(Equal("users/UnstoppableMango")) | ||
| }) | ||
|
|
||
| It("should stat user", func() { | ||
| fs := ghx.NewFs(client) | ||
|
|
||
| info, err := fs.Stat("users/UnstoppableMango") | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(info.Name()).To(Equal("UnstoppableMango")) | ||
| }) |
There was a problem hiding this comment.
These specs appear to make live GitHub API calls via client = internal.DefaultClient() (http.DefaultClient + optional GITHUB_TOKEN) but are not labeled E2E. Per AGENTS.md, non-E2E runs typically exclude E2E tests, so these will still run in make test and may be flaky/offline-failing. Consider labeling the network-dependent cases as Label("E2E") (or switching to an httptest/mock client) so unit test runs remain deterministic.
| // ReadAt implements afero.File. | ||
| func (f *File) ReadAt(p []byte, off int64) (int, error) { | ||
| if ra, ok := f.file.(io.ReaderAt); ok { | ||
| return ra.ReadAt(p, off) | ||
| } | ||
| return 0, errors.ErrUnsupported | ||
| } | ||
|
|
||
| // Readdir implements afero.File. | ||
| func (f *File) Readdir(count int) ([]fs.FileInfo, error) { | ||
| rd, ok := f.file.(fs.ReadDirFile) | ||
| if !ok { | ||
| return nil, errors.ErrUnsupported | ||
| } |
There was a problem hiding this comment.
ReadAt, Readdir*, and Seek return errors.ErrUnsupported when the wrapped fs.File doesn’t implement the corresponding optional interfaces. Elsewhere in this repo similar “unsupported” filesystem/file operations typically return syscall-style errors (e.g., syscall.EPERM/syscall.EROFS; see github/internal/readonly.go and writer/file.go). Returning a different sentinel error can be a breaking behavioral change for callers/tests that expect POSIX errors. Consider mapping these cases to the appropriate syscall errors for consistency (e.g., EPERM for seek/readat, ENOTDIR for readdir on non-directories, etc.).