Rename file system path types for v4 - #4260
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR renames the filesystem abstractions to ChangesFilePath and FolderPath migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR renames the public file-system path types to FilePath and FolderPath while updating consumers and migration documentation; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The pull request satisfies issue Full details: Docstring CoverageExplanation Docstring coverage is 20.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 248 functions across 58 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Inactive
Greptile SummaryThe PR performs the planned v4 rename of the public filesystem abstractions from
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines/FileSystem/FilePath.cs | Renames the file abstraction while preserving path operations, conversions, equality, and filesystem-provider propagation. |
| src/ModularPipelines/FileSystem/FolderPath.cs | Renames the folder abstraction and consistently updates folder composition, enumeration, and file access APIs. |
| src/ModularPipelines/Distributed/Serialization/ModuleResultSerializer.cs | Updates distributed serialization terminology and retains registration of portable path converters. |
| src/ModularPipelines/Distributed/Serialization/PortableFilePathJsonConverter.cs | Migrates portable file serialization from the removed file type to FilePath. |
| src/ModularPipelines/Distributed/Serialization/PortableFolderPathJsonConverter.cs | Migrates portable folder serialization from the removed folder type to FolderPath. |
| src/ModularPipelines/PublicAPI.Unshipped.txt | Records the renamed path API and the intentional removal of the former public types. |
| RELEASE_NOTES_V4.md | Documents the breaking path-type migration and clarifies that GetFile and GetFolder names remain unchanged. |
Reviews (16): Last reviewed commit: "fix(api): record renamed path removals" | Re-trigger Greptile
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Reviewed the full diff for "Rename file system path types for v4" (HEAD 464b5be6). This is a large, mostly mechanical rename of File/Folder to FilePath/FolderPath across the core library, tool integrations, tests, and docs, done to avoid the System.IO.File namespace collision described in #4212.
Verified:
- No leftover references to the old
ModularPipelines.FileSystem.File/Foldertype names remain insrc/ortest/(grepped forusing File = ModularPipelines.FileSystem.Fileand bare qualified references — all clear). GetFile/GetFoldermember names onIFilesContext/FolderPathare unchanged, matching the PR's stated intent.- The public API surface (implicit operators, equality members, constructors, JSON converters) was renamed consistently across both
FilePathandFolderPath.
Issue found
Mechanical rename leaked into a user-facing log message.
src/ModularPipelines/Context/Downloader.cs#L74 — the log message text changed from:
_moduleLoggerProvider.GetLogger().LogInformation("File {Uri} downloaded to {SaveLocation}", options.DownloadUri, filePathToSave);to:
_moduleLoggerProvider.GetLogger().LogInformation("FilePath {Uri} downloaded to {SaveLocation}", options.DownloadUri, filePathToSave);Here "File" was the English word in a sentence ("File downloaded to "), not a reference to the type name. The blanket find/replace turned this into log output like FilePath https://example.com/foo.zip downloaded to /tmp/xyz, which reads as broken grammar for every consumer of DownloadFileAsync. Notably, every other Log*Operation message in FilePath.cs/FolderPath.cs (e.g. "Creating File: {Path}", "Moving Folder: ...") was correctly left untouched, since those refer to the file-system concept rather than the type — which shows this is an isolated slip in the rename pass rather than an intentional style change.
Suggested fix: reword the log message so it no longer depends on the type name at all, e.g. "Downloaded {Uri} to {SaveLocation}". That also means the next time this type gets renamed, this log line won't drift into nonsense again.
No other correctness or CLAUDE.md compliance issues found — the rest of the change set is a clean, complete rename.
464b5be to
6b37925
Compare
|
Addressed the blocking review: restored the download log message to natural wording (Downloaded {Uri} to {SaveLocation}) while keeping the renamed FilePath type. Rebased onto current main. Validation: DownloaderTests passed (15/15). |
GitHub Actions
Failure
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 6b37925bb777df0419a817779aeb50b497d34396, up from the previously-reviewed 464b5be6.
Previous BLOCKING issue is fixed. The Downloader.cs log message ("File {Uri} downloaded to {SaveLocation}" → incorrectly "FilePath {Uri} downloaded...") has been reworded to "Downloaded {Uri} to {SaveLocation}", which sidesteps the type-name collision entirely. Good fix.
I re-read the full current diff (67 files) looking for the same class of mistake — places where the blanket File→FilePath / Folder→FolderPath find/replace leaked into English prose rather than an actual type reference — plus checked the core type definitions (FilePath.cs, FolderPath.cs, the JSON/portable converters) for any accidental logic changes hiding inside the rename. The rename is otherwise clean and consistent: no leftover bare File/Folder type references, no stale using File = ModularPipelines.FileSystem.File; aliases, equality/hashing/implicit-operator members and JSON converter logic are byte-identical apart from the type name, and GetFile/GetFolder member names were correctly left untouched.
Issue found
Doc comment leak, same class as the already-fixed Downloader.cs issue.
/// <summary>
/// FilePath checksum operations.
/// </summary>
IChecksumContext Checksum { get; }IChecksumContext computes a checksum of a file's contents — "File" here was the English word in "File checksum operations" (i.e. checksumming a file), not a reference to the File/FilePath type, the same way "File {Uri} downloaded..." wasn't a type reference in the previous round. The blanket rename turned this into "FilePath checksum operations", which reads as checksumming a path object rather than file contents — incorrect and slightly confusing in the public API docs.
Suggested fix: revert this doc comment to /// File checksum operations. (or reword to avoid the ambiguity entirely, e.g. /// Operations for computing file checksums.).
No other correctness issues found. Build of ModularPipelines.slnx (core) succeeds with 0 warnings/errors under TreatWarningsAsErrors.
GitHub Actions
Inactive
|
Addressed the current blocking review in |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 1ab56de3865c0577ac0e6df1426f622baf730ff9. This continues the File/Folder → FilePath/FolderPath rename (v4, ref #4212).
Both previous BLOCKING issues are confirmed fixed:
Downloader.cs:74log message is"Downloaded {Uri} to {SaveLocation}"— no longer leaks the type name into the sentence.IFilesContext.cs:57doc comment now reads/// Operations for computing file checksums.— no longer reads as "FilePath checksum operations."
I ran 8 parallel review passes across the diff (line-by-line read of all ~3168 diff lines plus a full build of ModularPipelines.slnx/ModularPipelines.Tests.slnf/the Git tool solution, a removed-behavior audit, a cross-file consumer/interface-consistency trace, and dedicated reuse/simplification/efficiency/altitude/CLAUDE.md-conventions passes). The rename is otherwise clean, complete, and consistent: no leftover ModularPipelines.FileSystem.File/Folder references anywhere in src//test/, no stale using File = ModularPipelines.FileSystem.File; aliases, no swapped File↔Folder identifiers, and all interface/implementation/JSON-converter/test-consumer pairs stay in sync.
New (minor) finding
Test files weren't renamed alongside the production types they test.
The production rename moved File.cs → FilePath.cs and Folder.cs → FolderPath.cs (proper git renames, tracked). But the matching test files kept their old names even though their content now exercises the renamed types:
test/ModularPipelines.UnitTests/FileSystem/FileTests.cs(testsFilePath)test/ModularPipelines.UnitTests/FileSystem/FileProviderTests.cstest/ModularPipelines.UnitTests/FileSystem/FolderTests.cs(testsFolderPath)test/ModularPipelines.UnitTests/FileSystem/FolderProviderTests.cstest/ModularPipelines.UnitTests/Extensions/FileExtensionsTests.cs
This breaks the filename symmetry the rest of the codebase relies on for navigating from a type to its tests — "find the tests for FilePath.cs" no longer resolves to FilePathTests.cs the way it does for every other type in this PR (e.g. TempFile.cs/TempFolder.cs kept their names because they weren't renamed, but every renamed production file should have its test file renamed too for the pattern to hold). Suggest renaming these five files (FileTests.cs→FilePathTests.cs, FileProviderTests.cs→FilePathProviderTests.cs, FolderTests.cs→FolderPathTests.cs, FolderProviderTests.cs→FolderPathProviderTests.cs, FileExtensionsTests.cs→FilePathExtensionsTests.cs) to close out the rename consistently — trivial to do and cheap to review since it's a pure git mv.
Cosmetic-only, not blocking on its own
test/ModularPipelines.Distributed.UnitTests/Serialization/PortablePathConverterTests.cs — the placeholder example path "src/MyProject/File.cs" got swept up in the blanket rename and became "src/MyProject/FilePath.cs" in three test cases. It's arbitrary test fixture data unrelated to the real FilePath type, so renaming it is harmless, but it's worth knowing this wasn't a deliberate choice — a future reader could mistake it for a reference to the real type. Not worth a separate fix if the file-renaming above is done in the same pass.
No correctness, design, or CLAUDE.md-compliance issues found beyond the above.
|
Rebased onto current main at 2f543a2; preserved both conflicting V4 release-note sections. Validation: Release core build 0 warnings/errors; FileTests 31/31; FolderTests 26/26. |
1ab56de to
2f543a2
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 2f543a21ca366945358d3c40a0017c3d0173ca66, which is a rebase onto current main (per the author's note: "Rebased onto current main at 2f543a2; preserved both conflicting V4 release-note sections"). I diffed the previously-reviewed commit (1ab56de386) against this HEAD directly to isolate what changed versus what's just base-branch churn from the rebase, and confirmed RELEASE_NOTES_V4.md ends up with a single, non-duplicated "File-system path types" section — the rebase did not introduce a merge-artifact duplication.
Both prior BLOCKING issues remain fixed:
Downloader.cslog message wording.IFilesContext.cschecksum doc comment wording.
The prior BLOCKING finding about test file naming is still not addressed.
The production rename moved File.cs → FilePath.cs and Folder.cs → FolderPath.cs as proper git renames, but the matching test files were not renamed to match, even though their content exercises the renamed types (confirmed unchanged in this HEAD — e.g. test/ModularPipelines.UnitTests/FileSystem/FileTests.cs still constructs new FilePath(...) under the old filename):
test/ModularPipelines.UnitTests/FileSystem/FileTests.cs(testsFilePath)test/ModularPipelines.UnitTests/FileSystem/FileProviderTests.cstest/ModularPipelines.UnitTests/FileSystem/FolderTests.cs(testsFolderPath)test/ModularPipelines.UnitTests/FileSystem/FolderProviderTests.cstest/ModularPipelines.UnitTests/Extensions/FileExtensionsTests.cs
This is a maintainability/naming-convention issue rather than a runtime bug: the rest of the codebase relies on the convention that a type's test file shares its name (e.g. TempFolder.cs ↔ TempFolderTests.cs), and this PR is the one place in the diff where that convention breaks — a future contributor searching for "the tests for FilePath.cs" won't find FilePathTests.cs. Since this PR already does a full mechanical rename pass (types, docs, JSON converters, log messages), leaving the test file names as the sole exception is inconsistent with the PR's own stated goal of a complete rename. Suggested fix, unchanged from the last round: git mv the five files to FilePathTests.cs, FilePathProviderTests.cs, FolderPathTests.cs, FolderPathProviderTests.cs, and FilePathExtensionsTests.cs. This is a trivial, low-risk, purely-mechanical follow-up (no code changes required beyond the git mv), so there's no reason to leave it as an exception to an otherwise complete rename.
No new correctness, design, or CLAUDE.md-compliance issues found in the rebase itself — the only outstanding item is the carry-over test-naming inconsistency above.
|
Addressed the remaining blocking Claude finding in 3805d34: renamed the five test files to FilePathTests.cs, FilePathProviderTests.cs, FolderPathTests.cs, FolderPathProviderTests.cs, and FilePathExtensionsTests.cs as content-identical git renames. Validation: FileTests pass 31/31. @claude please re-review the current head and update the verdict. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 3805d343423836cd259a2dd5f9552d95bcd19962, up from the previously-reviewed 2f543a21ca36.
The prior BLOCKING finding is now fixed. The five test files have been git mv'd to match the renamed production types, confirmed as pure 100%-similarity renames (no content changes):
FileTests.cs→FilePathTests.csFileProviderTests.cs→FilePathProviderTests.csFolderTests.cs→FolderPathTests.csFolderProviderTests.cs→FolderPathProviderTests.csFileExtensionsTests.cs→FilePathExtensionsTests.cs
This is the only change since the last-reviewed commit, so it closes out the naming-convention inconsistency without touching any other code.
All previously-flagged BLOCKING issues (the Downloader.cs log message, the IFilesContext.cs checksum doc comment, and this test-naming inconsistency) are now resolved. No new correctness, design, or CLAUDE.md-compliance issues found in this diff.
3805d34 to
2cdea71
Compare
|
Rebased onto current main after #4266. Resolved the two converter conflicts by preserving #4266 internal visibility while applying this PR's FilePath/FolderPath generic targets. Validation: ModularPipelines.slnx Release build 0 warnings/errors; FileTests 31/31; PublicSurfaceLeakageTests 3/3; git diff --check passed. @claude please re-review current head 2cdea71. |
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 2cdea7172191e344f651b285b6821726eecc60ab, which is a rebase of the previously-CLEARed 3805d343423836 onto current main after #4266 ("Reduce leaked core API surface"), per the author's note: "Rebased onto current main after #4266. Resolved the two converter conflicts by preserving #4266 internal visibility while applying this PR's FilePath/FolderPath generic targets."
All three prior BLOCKING findings remain fixed (confirmed unchanged in this diff):
Downloader.cslog message:"Downloaded {Uri} to {SaveLocation}".IFilesContext.cschecksum doc comment:"Operations for computing file checksums.".- Test files renamed to match production types (
FilePathTests.cs,FilePathProviderTests.cs,FolderPathTests.cs,FolderPathProviderTests.cs,FilePathExtensionsTests.cs).
Rebase conflict resolution verified. I diffed the merge base (79802137, i.e. #4266) against this HEAD to isolate exactly what #4266 touching the same files as this PR (the four JSON converters: PortableFilePathJsonConverter, PortableFolderPathJsonConverter, FilePathJsonConverter, FolderPathJsonConverter) resolved to. All four correctly combine both changes: internal visibility from #4266 is preserved, and the type/using changes from this PR (File/Folder → FilePath/FolderPath, dropping the using File = ModularPipelines.FileSystem.File; alias in favor of a plain using ModularPipelines.FileSystem;) are applied consistently. RELEASE_NOTES_V4.md also resolves to a single non-duplicated "File-system path types" section rather than a merge-artifact duplicate.
Full diff re-checked for the same class of issue that caused the earlier BLOCKING rounds (mechanical File→FilePath/Folder→FolderPath find/replace leaking into English prose rather than an actual type reference). Nothing new found — the diff is unchanged from the last CLEAR review except for the #4266 rebase mechanics above. One message worth explicitly confirming as not a regression of that pattern: FileExtensions.AssertExists now throws "FilePath reference is null{...}" (was "File reference is null{...}"). This reads correctly here — it's describing a null reference of the FilePath type, not the English word "file" — so it's the correct kind of rename, unlike the two that were previously flagged and fixed.
Also confirmed: no leftover ModularPipelines.FileSystem.File/Folder references anywhere in src//test/, and ModularPipelines.slnx (core) builds Release with 0 warnings/errors at this HEAD.
No correctness, design, or CLAUDE.md-compliance issues found. This PR is in a mergeable state.
GitHub Actions
Failure
2cdea71 to
c804fde
Compare
|
Rebased onto current Rebase validation:
The rebased branch also reconciles the API baselines with the generated-metadata namespace quarantine now on |
GitHub Actions
Failure
|
Fixed the post-#4251 CI failures in cd2d63e by updating all eight stale integration baseline references: DotNet File→FilePath, Git Folder→FolderPath, and GitHub File→FilePath. Validation under the exact CI analyzer gate: DotNet solution 0 errors (33 RS0026 advisories), Git solution 0 warnings/errors, GitHub solution 0 errors (3 existing test nullability warnings); no old FileSystem.File/Folder baseline references remain. @claude please re-review current head cd2d63e. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Review of #4260 (head cd2d63eb36c75c4b191517eae5f8114e13ac2365)
This is a large, almost entirely mechanical rename (File→FilePath, Folder→FolderPath). I traced usages across the whole repo (all tool-integration packages, Examples, docs, PublicAPI baselines) and the mechanical part of the rename is clean — no leftover bare references, no broken call sites, no tool package silently left on the old type. Two things flagged in earlier review rounds (the Downloader.cs log-wording revert and the Checksum XML-doc fix) are already addressed on this head, confirmed by the corresponding thomhurst comments and by reading the current file content.
Three issues remain, though, all introduced by this PR's own commits:
1. Test-file rename pass left the class names (and one sibling file) behind — test/ModularPipelines.UnitTests/FileSystem/
Commit "align path test filenames" renamed the files but not the types inside them, and missed one file entirely:
FilePathTests.csstill declarespublic class FileTestsFolderPathTests.csstill declaresclass FolderTestsFilePathProviderTests.csstill declaresclass FileProviderTestsFolderPathProviderTests.csstill declaresclass FolderProviderTestsFilePathExtensionsTests.csstill declaresclass FileExtensionsTeststest/ModularPipelines.UnitTests/Extensions/FolderExtensionsTests.cs— the structurally identical sibling ofFilePathExtensionsTests.cs— wasn't renamed at all
Filename/type-name mismatch defeats "go to definition"/"find by type name" workflows and test-explorer grouping, and the asymmetry between the File-side and Folder-side extension test files means whoever copies one as a template for the other will propagate the inconsistency. Since this is purely mechanical, a global rename of the five class declarations plus a git mv for FolderExtensionsTests.cs → FolderPathExtensionsTests.cs (with its class FolderExtensionsTests → FolderPathExtensionsTests) closes this out completely — no behavior to reason about, just finish the rename that was started.
2. Unrelated public-API change riding along in src/ModularPipelines/Generated/GeneratedCommandMetadata.cs
- bool isComplete = true)
+ bool isComplete)on Register(Type optionsType, IReadOnlyList<PropertyCommandLinePart> model, bool isComplete). No File/Folder type appears anywhere in this method's signature, so this isn't part of the rename — it's a separate, source-breaking public API edit (dropping a default parameter value) that happens to be sitting in the same commit as the baseline reconciliation for the rename. PublicAPI.Unshipped.txt was updated to match, so the analyzer won't catch it, but a reviewer scanning this as a "pure rename" PR would not expect to find an unrelated API-surface change here. Worth splitting into its own commit/PR with its own justification, or reverting if it wasn't intentional — bundling makes it too easy to wave through unreviewed.
3. Inconsistent PublicAPI baseline reconciliation between packages
For src/ModularPipelines, renamed symbols were correctly removed from PublicAPI.Shipped.txt and re-added to PublicAPI.Unshipped.txt (signaling "this shape hasn't shipped yet"). For src/ModularPipelines.Git, .DotNet, and .GitHub, the same renames (e.g. GitRepositoryInfo.Root.get -> ModularPipelines.FileSystem.Folder! → ...FolderPath!) were instead edited directly in place inside PublicAPI.Shipped.txt. That falsely asserts the new signature already shipped in a prior release of those packages. It doesn't fail the analyzer (which only diffs against current symbols), but it defeats the entire point of the Shipped/Unshipped split — namely, giving reviewers and consumers an accurate record of what changed since the last release for a breaking-change gate. Recommend applying the same Shipped→Unshipped move used for the core package to the Git/DotNet/GitHub baselines for consistency.
Minor, non-blocking: test/ModularPipelines.Distributed.UnitTests/Serialization/PortablePathConverterTests.cs renamed an arbitrary sample filename used purely as test fixture data ("src/MyProject/File.cs" → "...FilePath.cs") on both sides of the assertion. It's self-consistent so nothing breaks, but it's a sign the rename was done via blind text substitution rather than being type-aware — worth a quick check that no other literal strings were altered without reason.
|
Fixed the changed-file analyzer lane: FilePath.cs and FolderPath.cs now narrowly suppress pre-existing RS0026 overload warnings that became in-scope only because the files/types were renamed. Exact CI dotnet-format analyzer verification for both files passes. @claude please review the latest head. |
GitHub Actions
Failure
There was a problem hiding this comment.
Review
This is a mechanical rename PR (File/Folder → FilePath/FolderPath for v4), and the vast majority of it is clean and consistent — PublicAPI.Shipped.txt/Unshipped.txt reconciliation across core, DotNet, Git, and GitHub packages is complete and correctly paired, docs were updated, and the FolderPath/FilePath implicit string conversions keep dependent call sites (e.g. IEnvironmentContext.WorkingDirectory) compiling without changes.
Blocking
test/ModularPipelines.UnitTests/Helpers/FileInstallerTests.cs:9,18 — leftover reference to the removed ModularPipelines.FileSystem.File type:
using File = ModularPipelines.FileSystem.File;
...
var downloadedFile = new File("downloaded-installer");ModularPipelines.FileSystem.File was renamed to FilePath by this PR (see f198dfed refactor(files)!: rename path types), and every other identical alias site (e.g. src/ModularPipelines.Build/Helpers/NugetUploadHelper.cs) was updated to using ModularPipelines.FileSystem; + FilePath. This file was never touched by the rename commits — git log --follow on it only shows the earlier #4276 PR — so it still references a type that no longer exists anywhere in the repo. This will fail ModularPipelines.UnitTests compilation with CS0246 ("The type or namespace name 'File' could not be found"). Fix: change the alias to FilePath (or drop the alias and use ModularPipelines.FileSystem.FilePath directly) and update the constructor call to new FilePath("downloaded-installer").
Non-blocking suggestions
src/ModularPipelines/FileSystem/FilePath.cs(~lines 110, 158, 160, 174, 298, 326, 351, 382) andFolderPath.cs(~lines 321, 394, 452, 494): six separate#pragma warning disable RS0026 / restorepairs were added, each with a slightly different hand-written comment, all suppressing the same underlying reason (the v4 rename intentionally preserves overloads that would otherwise look ambiguous to the analyzer). Consider consolidating into a single suppression per file, or aGlobalSuppressions.cs/.editorconfigscoped severity override targetingFilePath/FolderPath— mirroring the existing single-pair precedent insrc/ModularPipelines/Context/IModuleContext.cs:191-208. A single suppression with one rationale is easier to keep correct than six near-duplicate comments that a future editor has to re-verify all mean the same thing.src/ModularPipelines/Context/Downloader.cs:74: the log message wording changed from"File {Uri} downloaded to {SaveLocation}"to"Downloaded {Uri} to {SaveLocation}". This is an unrelated cosmetic change riding along in a rename PR — not load-bearing (no test or known log-scraper asserts on the old text), but worth calling out or splitting into a separate commit if any external tooling scrapes these logs.- PR description says it "remove[s] obsolete type aliases" — no
[Obsolete]-attributedFile/Foldercompatibility shim was actually found in history; this line in the description doesn't correspond to any code being deleted. Minor description-accuracy nit only.
|
@claude review |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD ad5f570d4d240cdbb9a6595c2a48483ed0a6cb45, the diff since the last-reviewed d3127beb3dc962a2366276127c783c5fed664496 (single commit: test(files): use renamed file path type).
The prior BLOCKING issue is fixed. test/ModularPipelines.UnitTests/Helpers/FileInstallerTests.cs no longer aliases the removed ModularPipelines.FileSystem.File type — it now does using ModularPipelines.FileSystem; and constructs new FilePath("downloaded-installer"), matching the exact pattern used at the other identical alias site (src/ModularPipelines.Build/Helpers/NugetUploadHelper.cs). A repo-wide search confirms no remaining references to the removed FileSystem.File/FileSystem.Folder types.
No new issues introduced by this follow-up commit. This is a minimal, correctly-scoped fix — nothing further to flag.
|
Rebased onto current main after #4268 merged, preserving both release-note sections. Validation: core, DotNet, Git, and GitHub Release solution builds pass (GitHub retains 3 existing warnings); 68 focused tests pass; all 61 PublicAPI baseline pairs and assertion harness pass. |
ad5f570 to
5308aaa
Compare
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 5308aaaf424fef8f73ea861f008958271c29c217, up from the previously-CLEARed ad5f570d4d240cdbb9a6595c2a48483ed0a6cb45.
The branch was rebased onto current main (which since picked up #4268's ModuleResult metadata rename) between the two reviews, so the commit hashes diverged, but the content of the sole new commit (test(files): use renamed file path type) is byte-identical to what was already reviewed and cleared — confirmed with a direct diff of the two commits.
I isolated the actual PR diff against the current merge-base with main (73 files, matching the PR description) to exclude unrelated base-branch churn, and re-verified the full scope:
- No leftover bare references to the old
File/Foldertypes anywhere insrc/ortest/(GetFile/GetFoldermember names are correctly preserved per the design). - The two previously-fixed BLOCKING issues remain fixed:
Downloader.cslogs"Downloaded {Uri} to {SaveLocation}"(no type-name leak), andIFilesContext.Checksumstill documents "computing file checksums" (not "FilePath checksums"). PublicAPI.Shipped.txt/Unshipped.txtare correctly paired across the core, DotNet, Git, and GitHub packages — every removed old-type signature has a matching new-type signature added to Unshipped, and nothing outside theFile/Foldersurface was touched.RELEASE_NOTES_V4.mdhas a single, non-duplicated "File-system path types" section.- Files that overlap with #4268 (e.g.
ModuleResultSerializer.cs,GitInformation.cs,GitVersioning.cs) rebased cleanly — no conflict markers, no stale references.
No new issues. This PR is ready as-is.
GitHub Actions
Inactive
5308aaa to
f48ec28
Compare
|
Rebased onto current main through #4279/#4281. ZIP conflicts retain current CreateFromDirectory/ExtractToDirectory names with the renamed FilePath/FolderPath types; obsolete IFilesContext.Checksum stayed removed. Public API history now records all 121 shipped path removals and replaces four superseded unshipped ZIP declarations. Validation: ModularPipelines.Tests.slnf Release build succeeded (178 existing warnings, 0 errors); CI-conditioned core API build succeeded (6 advisory warnings, 0 errors); DotNet and Git solutions built with 0 warnings/errors; GitHub solution built with 3 existing warnings, 0 errors; 122 focused file/folder/ZIP/hash tests passed; 61 API baseline pairs passed; git diff --check passed. |
Reconcile the public API gate after the file-path rename and the generated metadata namespace quarantine merged into main.
Align test names and API baselines, remove broad analyzer suppressions, and preserve neutral fixture filenames.
f48ec28 to
c0c7b31
Compare
|
Rebased again after #4274 merged. Logging conflicts retain the new IModuleLoggerAccessor contracts while applying FolderPath construction and the generic Downloaded message. Validation on exact head: CI-conditioned core API build succeeded (6 advisory warnings, 0 errors); ModularPipelines.Tests.slnf Release build succeeded (178 existing warnings, 0 errors); DotNet/Git builds 0 warnings/errors; GitHub build 3 existing warnings, 0 errors; 42 focused ZIP/hash/downloader/logger/GitVersioning tests passed; 61 API baseline pairs passed; git diff --check passed. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD c0c7b3109c04a7cfac263d43a12eee0d19bd6b92, up from the previously-CLEARed 5308aaaf424fef8f73ea861f008958271c29c217.
All prior BLOCKING/minor findings from earlier rounds remain fixed:
Downloader.csstill logs"Downloaded {Uri} to {SaveLocation}"(no type-name leak into the log sentence).- The
IFilesContext.Checksumdoc-comment leak is moot —IChecksumContextitself was removed frommainby an unrelated, already-merged PR (IHashContextsupersedes it) before this branch rebased on top of it. - The five test files (
FileTests.cs,FileProviderTests.cs,FolderTests.cs,FolderProviderTests.cs,FileExtensionsTests.cs) were properlygit mv'd to theirFilePath/FolderPathnames.
What changed since the last CLEAR: the branch rebased onto a newer main that had merged unrelated work (a IZipContext method un-rename, IHasherContext→IHashContext, IModuleLoggerProvider→IModuleLoggerAccessor, PowerShell casing fixes). I isolated the actual diff of this PR (73 files, matching the PR description) against its current merge-base to separate this from base-branch churn — the only genuine new commit is fix(api): record renamed path removals, which only touches src/ModularPipelines/PublicAPI.Unshipped.txt to reconcile *REMOVED* entries for the old File/Folder surface after the rebase.
I verified that reconciliation directly:
- All 121
*REMOVED*entries in the corePublicAPI.Unshipped.txtreference only the oldFileSystem.File/FileSystem.Foldertypes (no unrelated removals slipped in), and every one has a matchingFilePath/FolderPathentry added elsewhere in the file. ModularPipelines.Git/ModularPipelines.DotNet/ModularPipelines.GitHubtook a different but equally valid approach (deleting the old signature fromShipped.txtoutright rather than recording a*REMOVED*marker inUnshipped.txt). To confirm this doesn't break the public API gate, I built all three affected tool solutions plus the core library locally with-p:EnableCiAnalyzers=true(mirroring the dedicated analyzer CI job) — all four build clean withRS0016/RS0017enforced as errors, confirming the baseline files are consistent with the actual compiled surface.
No new issues found. This PR is ready as-is.
1 participant
Summary
Validation
Closes #4212
Summary by CodeRabbit
FilePathandFolderPath.FileandFolderAPIs and platform-specific installer contexts.FailureMode,ExecutionHint, andPipelineBuilderSettings.