-
Notifications
You must be signed in to change notification settings - Fork 12
Fix MA0070: Add explanations to [Obsolete] attributes across the project #594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix MA0070: Add explanations to [Obsolete] attributes across the project #594
Conversation
Added descriptive messages to [Obsolete] attributes across the project to comply with MA0070. Updated OslcClient, ChangeRequest, OslcQuery, OslcMediaType, Requirement, and RequirementCollection. Ran dotnet format. Verified with dotnet build and tests. 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. |
WalkthroughAdds a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #594 +/- ##
=======================================
Coverage 50.86% 50.87%
=======================================
Files 174 174
Lines 10202 10204 +2
Branches 1010 1010
=======================================
+ Hits 5189 5191 +2
Misses 4756 4756
Partials 257 257 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OSLC4Net_SDK/OSLC4Net.Domains.RequirementsManagement/RequirementCollection.cs
Show resolved
Hide resolved
…esource - Added `Types` property to `AbstractResource` to support the recommendation. - Updated `[Obsolete]` messages in `ChangeRequest.cs` (both versions) to recommend using `.Types` instead of `GetRdfTypes`/`SetRdfTypes`/`AddRdfType`. - Deleted temporary build output file. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
OSLC4Net_SDK/OSLC4Net.ChangeManagement/ChangeRequest.cs (1)
126-129:⚠️ Potential issue | 🟡 Minor
AddRdfTypeis not marked[Obsolete]here but is in the client-sideChangeRequest.Per the relevant snippets,
OSLC4Net.Client/Oslc/Resources/ChangeRequest.csmarksAddRdfTypewith[Obsolete("Use .Types instead")]. For consistency, this method should also be deprecated.Suggested fix
+ [Obsolete("Use .Types instead")] public void AddRdfType(Uri rdfType) { AddType(rdfType); }
🧹 Nitpick comments (1)
OSLC4Net_SDK/OSLC4Net.Core/Model/AbstractResource.cs (1)
44-48: Consider adding a null guard on theTypessetter.Assigning
nullviaTypes = nullwould cause aNullReferenceExceptionon subsequent calls toAddType()(Line 109) orGetTypes(). While the existingSetTypes()method has the same gap, this new property is the recommended replacement going forward—worth making it defensive from the start.🛡️ Suggested fix
public ICollection<Uri> Types { get => types; - set => types = value; + set => types = value ?? throw new ArgumentNullException(nameof(value)); }Also, per Microsoft Framework Design Guidelines, collection properties generally should not have public setters (callers should use
Clear/Addon the existing collection instead of replacing it). However, since the existingSetTypesmethod follows this same pattern throughout the codebase, this is noted for awareness rather than as a blocking concern. As per coding guidelines, "Review the C# code against the Microsoft Framework design guidelines".
Removed build_output.txt which was accidentally included in the previous commit. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
Added `AbstractResourceTests.cs` to verify the functionality of the new `Types` property on `AbstractResource`. Verified that `Types` behaves consistently with `GetTypes`, `SetTypes`, and `AddType`. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/AbstractResourceTests.cs`:
- Around line 13-38: Split the single TestTypesProperty into multiple focused
tests each covering one behavior: create TestAddType_UpdatesTypes to exercise
ConcreteResource.AddType and assert Types contains the new Uri and count
increments; TestGetTypes_ReturnsSameCollectionAsTypes to assert GetTypes()
returns the same collection instance/value as the Types property;
TestTypeSetter_ReplacesCollection to set Types to a new List<Uri> and assert old
entries are removed and new entries present; TestSetTypes_UpdatesTypes to call
SetTypes and assert Types equals the provided list. Additionally add edge-case
tests: TestInitialTypes_IsEmpty to verify initial Types state,
TestAddType_DuplicateDoesNotCreateExtra (or documents expected duplicate
behavior) to assert adding a duplicate type behaves as intended, and
TestSetTypes_EmptyListClearsTypes to set Types to an empty list and verify Types
is empty; reference ConcreteResource, AddType, GetTypes, Types (getter/setter)
and SetTypes when implementing the new test methods to improve branch coverage.
🧹 Nitpick comments (1)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/AbstractResourceTests.cs (1)
7-11: Unused constructorConcreteResource(Uri about).The
Uri-accepting constructor is never exercised in this test class. Consider removing it to keep the test helper minimal, or add a test that uses it (e.g., verifyingAboutis set correctly).
Refactored TestTypesProperty into multiple focused tests. Added checks for round-tripping capability (verifying attributes). Fixed usage of OslcPropertyDefinition attribute in tests. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
- Refactored `AbstractResourceTests.cs` to test `Types` property behaviors individually. - Added `AbstractResourceSerializationTests.cs` to verify round-trip serialization of `Types` using `OSLC4Net.Core.DotNetRdfProvider`. - Ensured proper RDF mapping attributes are verified. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
Added verification that the type URI from [OslcResourceShape] is present in the deserialized object in AbstractResourceSerializationTests. Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
This PR addresses the compiler warning
MA0070: Obsolete attributes should include explanations.It adds explanatory messages to
[Obsolete]attributes in several files:OSLC4Net.Client/Oslc/OslcClient.cs: Added messages suggesting replacements likeHttpClient,DeleteResourceAsync,UpdateResourceRawAsync.OSLC4Net.Client/Oslc/Resources/ChangeRequest.cs: Added message toAddRdfType.OSLC4Net.Client/Oslc/Resources/OslcQuery.cs: Added message toGetQueryUrl.OSLC4Net.ChangeManagement/ChangeRequest.cs: Added messages toGetRdfTypesandSetRdfTypes.OSLC4Net.Core/Model/OslcMediaType.cs: Added messages to obsolete media type constants.OSLC4Net.Domains.RequirementsManagement/Requirement.csandRequirementCollection.cs: Added messages to obsoleteTypeproperty.No new warnings were introduced. Code style enforced with
dotnet format. Existing tests passed.PR created automatically by Jules for task 1406957271129390419 started by @berezovskyi
Summary by CodeRabbit
New Features
Chores
Tests