core: fix MA0140 duplicate branches in ServiceProviderFactory#785
core: fix MA0140 duplicate branches in ServiceProviderFactory#785berezovskyi wants to merge 1 commit into
Conversation
Extracted the common `returnUri` assignment out of the `if` and `else` branches in the `ServiceProviderFactory.ResolvePathParameters` method. The redundant conditional structure generated an MA0140 warning due to identical code in both branches. Preserved the TODO comment structure. Ran `dotnet format style` on the solution. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. WalkthroughThis PR refactors the conditional logic in ChangesPath Resolution Simplification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Code Coverage OverviewLanguages: C# C# / dotnet/coberturaThe overall coverage remains at 55%, unchanged from the branch. Code Coverage is in Public Preview. Learn more and provide us with your feedback. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
OSLC4Net_SDK/OSLC4Net.Core/Model/ServiceProviderFactory.cs (1)
368-381: ⚡ Quick winGood refactoring to eliminate duplication, but update the comment to reflect current behavior.
The MA0140 fix correctly consolidates the duplicate assignment. However, the comment on line 380 is now misleading—it describes this as "Fallback or no parameters supplied," but since templated path resolution is not yet implemented (the TODO block doesn't assign
returnUri), this code always executes regardless of whether parameters are supplied.♻️ Proposed fix to clarify the comment
- // Fallback or no parameters supplied - assume `@Path` not templated + // Current implementation: templated path resolution not yet supported - assume `@Path` not templated returnUri = basePath + "/" + pathAttribute;Alternatively, consider simplifying the method structure since the conditional currently adds no functional value:
private static string ResolvePathParameters(string basePath, string pathAttribute, Dictionary<string, object> pathParameterValues) { - string returnUri; - - //Build the path from the `@Path` template + map of parameter value replacements - if (pathParameterValues != null && pathParameterValues.Count > 0) - { - /* TODO - not supported yet - UriBuilder builder = UriBuilder.fromUri(basePath); - URI resolvedUri = builder.path(pathAttribute).buildFromMap(pathParameterValues); - if (resolvedUri != null) - { - returnUri = resolvedUri.tostring(); - } - */ - } - - // Fallback or no parameters supplied - assume `@Path` not templated - returnUri = basePath + "/" + pathAttribute; - - return returnUri; + /* TODO: Build the path from the `@Path` template + map of parameter value replacements + * Currently not supported - templated path resolution would use pathParameterValues here + * Example: UriBuilder.fromUri(basePath).path(pathAttribute).buildFromMap(pathParameterValues) + */ + + // Current implementation: assume `@Path` is not templated + return basePath + "/" + pathAttribute; }As per coding guidelines, this aligns with Building Maintainable Software principle "Write Simple Units of Code" by removing the conditional structure that adds no current value while preserving the TODO for future implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OSLC4Net_SDK/OSLC4Net.Core/Model/ServiceProviderFactory.cs` around lines 368 - 381, The if (pathParameterValues != null && pathParameterValues.Count > 0) block currently has no effect because templated path resolution is not implemented; update the code by removing that no-op conditional (or at minimum collapsing it) and replace the misleading comment with a clear note that templated `@Path` resolution is not yet supported and the method always falls back to constructing returnUri as basePath + "/" + pathAttribute; reference the symbols pathParameterValues, returnUri, pathAttribute and basePath in your change and keep the existing TODO about adding templated resolution later.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@OSLC4Net_SDK/OSLC4Net.Core/Model/ServiceProviderFactory.cs`:
- Around line 368-381: The if (pathParameterValues != null &&
pathParameterValues.Count > 0) block currently has no effect because templated
path resolution is not implemented; update the code by removing that no-op
conditional (or at minimum collapsing it) and replace the misleading comment
with a clear note that templated `@Path` resolution is not yet supported and the
method always falls back to constructing returnUri as basePath + "/" +
pathAttribute; reference the symbols pathParameterValues, returnUri,
pathAttribute and basePath in your change and keep the existing TODO about
adding templated resolution later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ccc958b-dbf3-4453-8f6e-8a23c0618258
📒 Files selected for processing (1)
OSLC4Net_SDK/OSLC4Net.Core/Model/ServiceProviderFactory.cs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #785 +/- ##
=======================================
Coverage 51.26% 51.27%
=======================================
Files 174 174
Lines 10281 10280 -1
Branches 1057 1057
=======================================
Hits 5271 5271
+ Misses 4750 4749 -1
Partials 260 260 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
berezovskyi
changed the title
1 participant
Refactor ResolvePathParameters to resolve MA0140 warning
Extracted the common
returnUriassignment out of theifandelsebranches in theServiceProviderFactory.ResolvePathParametersmethod. The redundant conditional structure generated an MA0140 warning due to identical code in both branches.Preserved the TODO comment structure.
Ran
dotnet format styleon the solution.PR created automatically by Jules for task 17216203223603717034 started by @berezovskyi
Summary by CodeRabbit