Fix LINQ missed-where warnings, cognitive complexity violations, Assert.IsTrue code smells, and linter design doc#137
Conversation
…mells Agent-Logs-Url: https://github.com/demaconsulting/ReqStream/sessions/b1924eda-db55-4fc3-bdec-bae61fed401d Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
|
@copilot Is the linter design documentation still good, or does it need updates? |
There was a problem hiding this comment.
Pull request overview
This PR improves code quality by addressing static analysis findings in the linter and test suite, primarily around LINQ filtering patterns, cognitive complexity thresholds, and MSTest assertion usage.
Changes:
- Updated integration tests to use
Assert.Contains(...)instead ofAssert.IsTrue(x.Contains(y)). - Refactored
Linter.csto reduce cognitive complexity by extracting helper methods (includes, document sections/mappings, section requirements/children, requirement id/title). - Replaced implicit filtering inside
foreachloops with explicit LINQ pipelines and simplifiedGetStringListinto a single LINQ expression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/DemaConsulting.ReqStream.Tests/IntegrationTests.cs | Replaces Assert.IsTrue(...Contains...) assertions with Assert.Contains(...) for analyzer compliance and consistency. |
| src/DemaConsulting.ReqStream/Linter.cs | Extracts helper methods to reduce cognitive complexity and adjusts iteration/filtering patterns to satisfy LINQ analyzers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Agent-Logs-Url: https://github.com/demaconsulting/ReqStream/sessions/b496bf5d-4f9f-456f-a5a3-e9c5e2fd1f0c Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
The doc needed updates — it referenced three methods ( Updated in 93ce05b:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… message context Agent-Logs-Url: https://github.com/demaconsulting/ReqStream/sessions/6c19a1b4-bf3c-471d-9c55-8f0172f73063 Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Addresses four categories of static analysis violations in
Linter.csandIntegrationTests.cs: implicit sequence filtering inforeachloops, cognitive complexity exceeding the S3776 threshold, and use ofAssert.IsTrue(x.Contains(y))instead ofAssert.Contains. Also updates the linter unit design documentation to match the refactored method structure, and improves error message context for duplicate requirement IDs.Description
IntegrationTests.cs— MSTEST0037Assert.IsTrue(x.Contains(y))withAssert.Contains(y, x)on 4 assertions, consistent with existing patterns in the test suite.Linter.cs—cs/linq/missed-where(lines 454, 469, 528, 615)iffilters insideforeachwith explicit.OfType<>().Where().Select()chains.GetStringListto a single LINQ expression eliminating the loop entirely.Linter.cs— S3776 Cognitive ComplexityExtracted 7 focused helper methods to bring all four violating methods within the limit of 15:
LintFileLintIncludesLintDocumentRootLintDocumentSections,LintDocumentMappingsLintSectionLintSectionRequirements,LintSectionChildrenLintRequirementLintRequirementId,LintRequirementTitleLinter.cs—LintRequirementIdduplicate-ID error contextLintRequirementIdnow returnsreqId(instead ofnull) in the duplicate-ID case. The ID is still not added toseenIds, and the issue count is still incremented. This allowsLintRequirementTitleto produce the more actionable "requirement 'REQ-123' missing required field 'title'" message rather than the generic fallback, with no cascading side effects.docs/design/linter.md— Design documentation updateThe linter unit design document previously referenced three method names (
LintSections,LintRequirements,LintMappings) that were aspirational design names and never existed in the code. Updated to reflect the actual implementation:LintIncludes,LintDocumentSections,LintDocumentMappings,LintSectionRequirements,LintSectionChildren,LintRequirementId, andLintRequirementTitle.LintFile,LintDocumentRoot,LintSection,LintRequirement, andLintMappingto correctly describe their delegation to the new helpers.LintRequirementIdstep 3 to reflect the correctedreqIdreturn in the duplicate case.Type of Change
Pre-Submission Checklist
Before submitting this pull request, ensure you have completed the following:
Build and Test
dotnet build --configuration Releasedotnet test --configuration Releasedotnet run --project src/DemaConsulting.ReqStream --configuration Release --framework net10.0--no-build -- --validateCode Quality
dotnet format --verify-no-changesQuality Checks
Please run the following checks before submitting:
cspell "**/*.{md,cs}"markdownlint "**/*.md"yamllint .Testing
Documentation
Additional Notes
All 170 tests pass across .NET 8, 9, and 10. CodeQL scan shows 0 alerts. Spell check and markdown lint pass on the updated design document.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.