Fix errors/warnings for pre-existing code smells after reintroducing editorconfig et al - #393
Merged
Merged
Conversation
ChrisonSimtian
added
enhancement
ChrisonSimtian
changed the title
ChrisonSimtian
added a commit
that referenced
this pull request
Jun 18, 2026
Aligns the MSBuild settings type with the project's PascalCase-acronym convention (MsBuild, not MSBuild), matching the surrounding API surface. Also drops an unused PropertyInfo parameter from the FormatPlatform formatter. Split out of #393 so the rename can be reviewed on its own. BREAKING: MsBuildSettings is public API and this rename ships no transition shim, so it is a hard break. It targets experimental and is batched to the yearly major cut per the channel model (CLAUDE.md rule #1). CHANGELOG updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChrisonSimtian
added a commit
that referenced
this pull request
Jun 18, 2026
Aligns the MSBuild settings type with the project's PascalCase-acronym convention (MsBuild, not MSBuild), matching the surrounding API surface. Split out of #393 so the rename can be reviewed on its own. BREAKING: MsBuildSettings is public API and this rename ships no transition shim, so it is a hard break. It targets experimental and is batched to the yearly major cut per the channel model (CLAUDE.md rule #1). CHANGELOG updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dennisdoomen
force-pushed
the
chore/fix-csharp-style-warnings
branch
3 times, most recently
from
June 20, 2026 07:15
f6b86ed to
a15bf8f
Compare
ChrisonSimtian
force-pushed
the
chore/fix-csharp-style-warnings
branch
from
June 20, 2026 07:24
a15bf8f to
bc09229
Compare
dennisdoomen
previously approved these changes
Jun 20, 2026
dennisdoomen
left a comment
dennisdoomen
left a comment
Collaborator
There was a problem hiding this comment.
A suggestion and a verification
Surfaced by re-enabling the editorconfig. Across the build utilities, CLI commands, and tool infrastructure, method signatures declared parameters that were never read by the implementation. Each call site passed values that went unused, and removing them makes the intent of each method clear. Areas touched: ControlFlow suppression helpers (includeStackTrace), SchemaUtility enumeration schema builder (SchemaContext), CLI navigation and setup/update commands (redundant path and project-name arguments), GitHubActionsAttribute step builder (GitHubActionsImage), CodeGenerator runtime information applicator (specificationFile, sourceFileProvider, namespaceProvider), ProcessTasks invocation logger (hasEnvironmentVariables). One unused `using System.IO` import and a stale comment in the splash screen are also cleaned up.
…lizer The generic arity suffix on the base class name was noise — there is only one concrete base, so the backtick-1 form added no information. Renaming to SingleFileSerializerBase makes the type easier to reference and matches the naming of the interface it implements. The file-stream handling switches from `await using` to synchronous `using`. `FileStream` does not implement `IAsyncDisposable` on `netstandard2.0`, so the async disposal form failed to compile there (CS8417). Synchronous disposal of a local read/write stream is correct on all target frameworks. Also fixed: a namespace mismatch on ISerializerModelExtension, two doc-comment typos in ISolutionSerializer, and redundant lambda parameter names in XmlFolder.
Formatter methods were always invoked with two arguments (value, PropertyInfo), requiring every formatter to declare a PropertyInfo parameter even when it didn't use it. Removing that unused parameter then caused a parameter-count mismatch at runtime, surfacing as test failures once the CorFlagsSettings formatter was cleaned up. The invoker now matches arguments to the formatter's declared arity: single-parameter formatters receive [value] only; two-parameter formatters still receive [value, PropertyInfo]. Existing formatters are unaffected; formatters that don't need reflection context can now omit the PropertyInfo parameter.
…-arity nitpick Reverts the inadvertent namespace move of the public ISolutionSerializer interfaces (ISolutionSerializer, ISolutionSerializer<TSettings>, ISolutionSingleFileSerializer<TSettings>) from Fallout.Persistence.Solution to .Serializer. The move was an IDE-applied "namespace matches folder" tidy that slipped into this otherwise non-breaking style-cleanup PR. It is a breaking change to public API (per @dennisdoomen's review) and also diverges from the upstream vs-solutionpersistence layout, which deliberately keeps the primary interface in the root namespace while concrete serializers live under .Serializer. Restoring the original namespace also drops the now-unneeded using in ISerializerModelExtension.cs (.Model is a child namespace and sees the parent's types without an explicit using). Also tighten the formatter-arity check to `== 1` (a formatter always takes at least the value argument, so a length of 0 is not a valid shape). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChrisonSimtian
force-pushed
the
chore/fix-csharp-style-warnings
branch
from
June 22, 2026 07:32
014c79d to
76c552a
Compare
ChrisonSimtian
added a commit
that referenced
this pull request
Jun 22, 2026
…d/ folder Makes accessibility explicit on shipped/tested surface — no behavioural change: - Component interfaces (IFalloutBuild, ICreateGitHubRelease, IHasTwitterCredentials, ISignPackages, IToolOptionsWithFramework): drop the redundant `public` from interface members (implicitly public; matches the for_non_interface_members rule). - Consumer/sample builds under tests/ and DefaultInterfaceExecutionTest: explicit internal/private. The dogfood build/ folder is deliberately exempted rather than encapsulated: a [build/**.cs] editorconfig section relaxes dotnet_style_require_accessibility_modifiers there, so build scripts stay terse (convenience over ceremony, per @dennisdoomen's "never bothered with visibility in builds" note). The explicit modifiers this PR originally added under build/ are dropped to match. Split out of #393. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dennisdoomen
approved these changes
Jun 22, 2026
ChrisonSimtian
added a commit
that referenced
this pull request
Jun 22, 2026
…d/ folder Makes accessibility explicit on shipped/tested surface — no behavioural change: - Component interfaces (IFalloutBuild, ICreateGitHubRelease, IHasTwitterCredentials, ISignPackages, IToolOptionsWithFramework): drop the redundant `public` from interface members (implicitly public; matches the for_non_interface_members rule). - Consumer/sample builds under tests/ and DefaultInterfaceExecutionTest: explicit internal/private. The dogfood build/ folder is deliberately exempted rather than encapsulated: a [build/**.cs] editorconfig section relaxes dotnet_style_require_accessibility_modifiers there, so build scripts stay terse (convenience over ceremony, per @dennisdoomen's "never bothered with visibility in builds" note). The explicit modifiers this PR originally added under build/ are dropped to match. Split out of #393. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChrisonSimtian
added a commit
that referenced
this pull request
Jun 22, 2026
…d/ folder Makes accessibility explicit on shipped/tested surface — no behavioural change: - Component interfaces (IFalloutBuild, ICreateGitHubRelease, IHasTwitterCredentials, ISignPackages, IToolOptionsWithFramework): drop the redundant `public` from interface members (implicitly public; matches the for_non_interface_members rule). - Consumer/sample builds under tests/ and DefaultInterfaceExecutionTest: explicit internal/private. The dogfood build/ folder is deliberately exempted rather than encapsulated: a [build/**.cs] editorconfig section relaxes dotnet_style_require_accessibility_modifiers there, so build scripts stay terse (convenience over ceremony, per @dennisdoomen's "never bothered with visibility in builds" note). The explicit modifiers this PR originally added under build/ are dropped to match. Split out of #393. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes pre-existing C# warnings / code smells surfaced after reintroducing
.editorconfig(#388).Scope narrowed per @dennisdoomen's review — the two larger, opinion-dependent changes were split out so this PR stays focused and reviewable:
MSBuildSettings→MsBuildSettingsrename → separate PR (pending, breaking-change handling TBD)What remains here
Remove unused method parameters and usings across utilities and CLI
ControlFlow,SchemaUtility,CodeGenerator,ProcessTasks,GitHubActionsAttribute, and the CLI navigation/setup/update commands all had method signatures declaring parameters that were never read. Removed the dead parameters and one unusedusingimport. A stale splash-screen comment is also corrected.Introduce
SingleFileSerializerBaseand clean up the persistence serializerThe generic arity suffix on the base class name was noise (there is only one concrete base), so it is renamed to
SingleFileSerializerBase. Theawait using FileStreampattern is replaced with synchronoususingto fix anetstandard2.0compile break (FileStreamis notIAsyncDisposablethere — CS8417). Also fixed: a namespace mismatch onISerializerModelExtension, two doc-comment typos inISolutionSerializer, and redundant lambda parameter names inXmlFolder.Make
PropertyInfoargument optional for tool-option formattersFormatter methods were always invoked with two arguments (
value,PropertyInfo), forcing every formatter to declare thePropertyInfoparameter even when unused. Removing it inCorFlagsSettings(as part of the unused-parameter cleanup above) caused a parameter-count mismatch at runtime. The invoker now matches arguments to the formatter's declared arity, making thePropertyInfoparameter genuinely optional without breaking existing two-parameter formatters.Verification
Full
dotnet build fallout.slnxsucceeds (0 errors).