PR 3: chore(quality): mechanical analyzer-driven cleanup (453 files)#176
Open
rlorenzo wants to merge 8 commits into
Open
PR 3: chore(quality): mechanical analyzer-driven cleanup (453 files)#176rlorenzo wants to merge 8 commits into
rlorenzo wants to merge 8 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 300 files, which is 150 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (300)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
0d26c81 to
438134a
Compare
rlorenzo
force-pushed
the
tools/codeql-roslyn-sonar-resharper
branch
2 times, most recently
from
078b830 to
ccde16a
Compare
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
438134a to
11799f3
Compare
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
11799f3 to
2580a27
Compare
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 42.95% 43.01% +0.05%
==========================================
Files 876 876
Lines 51454 51399 -55
Branches 4802 4808 +6
==========================================
+ Hits 22101 22108 +7
+ Misses 28829 28767 -62
Partials 524 524
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
2 times, most recently
from
6b500eb to
f9e9c1a
Compare
- Bulk-applied via `jb cleanupcode --profile=OptimizeUsings` then `dotnet format`. Removes unused usings and sorts/groups remaining ones — compiler-equivalent, no behavior change. - Adds Viper.sln.DotSettings with OptimizeUsings, ShortenReferences, and RemoveRedundancies profiles for this and follow-up PRs. - Reproduction note: cleanupcode deadlocks on MSBuild's shared compiler server while the dev server runs; set DOTNET_USE_COMPILER_SERVER=0 to bypass.
- Bulk-applied via `jb cleanupcode --profile=ShortenReferences` then `dotnet format`. Replaces inline `Namespace.Type` with imported `Type` (or `using Alias = Namespace.Type;` for ambiguous names). Compiler-equivalent — no behavior change. - PhotoExportService.cs gets the largest single diff because it juggles four DocumentFormat.OpenXml sub-namespaces with conflicting type names (`Picture`, `Paragraph`, `Run`, etc.); aliases at the top replace the inline full qualifiers throughout.
- Drop dead `?.` and `!= null` guards in 10 files where the analyzer proves the value is non-null (DI-injected helpers, catch parameters, values guaranteed by preceding null-checks). - ViteProxyHelpers.cs: suppress CA1508 with `#pragma warning disable` on the inner check of a double-checked locking pattern. The outer guard may have raced; CA1508's dataflow analysis doesn't model thread interleaving, so this is a known false positive.
- Strip empty `<param name="X"></param>` tags whose corresponding parameter doesn't exist (or where the tag adds no doc value). Removes ~24 InvalidXmlDocComment findings. - Fix malformed XML: escape literal `<T>` as `<T>` in three doc comments (HttpHelper, IamApi, UinformService); promote two `// <summary>` typos in IamApi to `///`; demote orphan `///` block on Program.cs SetAwsCredentials (top-level local function doesn't accept doc comments) to plain comment. - Rename three orphan `<param>` tags to match real parameters (UserHelper.HasPermission, RAPSSecurityService.IsAllowedTo, VMACSExport.ExportToInstances). Partial-coverage cases (methods with descriptive `<param>` tags for some but not all parameters) intentionally left alone — their existing documentation has real value, and selectively adding empty tags or deleting good ones would be a net loss.
Fixes the 29 PossibleMultipleEnumeration findings flagged by ReSharper. Each call site enumerated an IEnumerable two-or-more times (e.g., `source.Any()` then `source.First()`, or `source.Sum(a)` then `source.Sum(b)`). Materialising once with `.ToList()` is semantically equivalent and avoids re-evaluating deferred queries. - Test files: cast assertion targets to lists so xUnit's `Assert.NotEmpty` + `Assert.Equal(N, x.Count())` doesn't iterate twice. - VueTableDefault.cs: `data`, `skipColumns`, `altColumnNames` are enumerated 3+ times per helper and 3 times across helpers from `InvokeAsync`. Materialise at both layers. - EvaluationPolicyService.IsRequiredFor*: `rotationWeeks` is enumerated by `.Any()`, `.FirstOrDefault(currentWeek)`, and `.FirstOrDefault(nextWeek)`. Materialise at top of method. - CMSContentController.GetContentBlockByFn: switch `Any()/First()` to `Count == 0` / indexer. - CliniciansController.FilterCliniciansByPermissions: rename param to `cliniciansSource`, materialise once into `clinicians`. - Smaller fixes in RotationsController, EvaluationReportService.
- Bulk-applied via `jb cleanupcode --profile=RemoveRedundancies` then `dotnet format`. Removes redundant argument default values, redundant member initializers, redundant `else` after `return`, redundant anonymous-type property names, and other rewrites in the ReSharper "Remove code redundancies" category — all compiler-equivalent, no behavior change. - Updates `Viper.sln.DotSettings`: the umbrella `<CSRemoveCodeRedundancies>` flag alone is a no-op. ReSharper also requires the (sparsely-documented) sibling `<RemoveCodeRedundancies>` flag plus per-rule sub-flags (`<CSRemoveRedundantArgumentDefaultValues>`, `<CSRemoveRedundantInitializers>`) to actually trigger the rewrites. - Fixes a minor follow-up: `CliniciansController.cs:569` was changed to `clinicians.Count` (List property) from `clinicians.Count()` (LINQ). The IEnumerable to List materialisation in `9823a4c9` made `clinicians` a List, which Sonar S2971 then flagged as preferring the property. No new lint rule violations introduced by this commit.
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
2 times, most recently
from
55cbcab to
7f36555
Compare
- Real cleanups across CMS / ClinicalScheduler / Effort / RAPS / Students: drop dead null-checks Roslyn flow analysis can prove unreachable, collapse a duplicated return path, simplify the cache lookup that was capturing a never-assigned outer variable, fix the stale XML doc param tag left behind by the IEnumerable materialisation rename, remove a virtual call from a DTO constructor, and split a SqlCommand object initializer outside the using statement. - Replace anonymous-type byte[]? gymnastics in PhotoExportService with a named record so CodeQL stops flagging the casts as redundant. - Gate now supports `--exclude-rule` and ships defaults for rules that fire false positives on ASP.NET Core / EF surfaces (DTO accessors bound at runtime by JSON / MVC, record positional properties used via record pattern Equals, and the EF nav-property NRT contract that Roslyn intentionally distrusts because Include() can be missing).
The UserHelper.GetByLoginId cache populator doesn't need the ICacheEntry parameter; using `_` silences the new ReSharper UnusedParameter.Local finding the previous commit introduced when it collapsed the closure.
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
7f36555 to
918ce04
Compare
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
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.
Summary
Part 3 of 6. Stacks on top of PR #175. This is the elephant — 453 files changed, but they're almost all mechanical, tool-applied rewrites with no behavior change.
Commits (each is one analyzer rule's bulk fix)
optimize using directivesshorten reference qualifiersNamespace.Type→ importedTyperemove dead null-checksclean up stale XML doc commentsmaterialize IEnumerableremove code redundanciesjb cleanupcode --profile=RemoveRedundanciesclear ReSharper PR-gate + CodeQL review findingsdiscard unused entry paramWhy bundled
Splitting these by area would create artificial PRs that the reviewer still couldn't read line-by-line. They share the same review style — "trust the tool, spot-check the result" — so they're better together.
Conflict resolution notes
Two cherry-pick conflicts hit because later refactors (in PR #5/#6) modified files this PR also touches:
web/Areas/CMS/Data/Codecs.cs(refactored later in PR 6) — applied just theSystem.IO.Stream→Streamqualifier shortening to the pre-refactor file.web/Areas/Students/Services/StudentGroupService.cs(refactored later in PR 6) — applied just theid => id != null→!string.IsNullOrEmpty(id)and SqlException qualifier fixes; skipped thesealed recordchange which targets a record introduced by the PR 6 refactor.web/Areas/Effort/Services/EvaluationReportService.cs: skipped — thematerialize IEnumerablefix targetsCalculateWeightedAverage, a helper that gets introduced by PR 5. The fix is moved into PR 5 atop that extraction.PR stack
Test plan
npm run verify:buildsucceeds locallynpm run testpasses