feat: implement IPredecessorFindable interface for Repository class#357
feat: implement IPredecessorFindable interface for Repository class#357wangxiaoxuan273 wants to merge 2 commits intooras-project:mainfrom
Conversation
Signed-off-by: Xiaoxuan Wang <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
==========================================
+ Coverage 91.76% 91.78% +0.02%
==========================================
Files 64 64
Lines 2755 2764 +9
Branches 364 365 +1
==========================================
+ Hits 2528 2537 +9
Misses 138 138
Partials 89 89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements the IPredecessorFindable interface (via IReadOnlyGraphStorage) on the concrete Repository class, enabling it to be used with ExtendedCopyGraphAsync for extended copy operations that traverse the artifact graph through referrers.
Changes:
Repositoryclass now declaresIReadOnlyGraphStoragein addition toIRepositoryon its class declaration.- A new public
GetPredecessorsAsyncmethod is added toRepository, delegating to the existingFetchReferrersAsync(which handles both the Referrers API and tag schema fallback). - Two new unit tests cover the happy path (referrers returned) and empty-referrers cases for
GetPredecessorsAsync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/OrasProject.Oras/Registry/Remote/Repository.cs |
Adds IReadOnlyGraphStorage to Repository's interface list and implements GetPredecessorsAsync by delegating to FetchReferrersAsync. |
tests/OrasProject.Oras.Tests/Registry/Remote/RepositoryTest.cs |
Adds two tests for GetPredecessorsAsync: one with referrers, one with an empty referrers list. |
You can also share your feedback on Copilot code review. Take the survey.
- Add <remarks> XML doc to Repository class explaining that IRepository does not extend IReadOnlyGraphStorage and callers must cast to access graph-related APIs - Use interface types (IPredecessorFindable/IReadOnlyGraphStorage) instead of var for repo in tests per codebase conventions - Add test for referrers API error path (ResponseException) - Add test for tag schema fallback path when referrers API is not supported Signed-off-by: Xiaoxuan Wang <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
|
Caution This review was generated by Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT. Review of PR #357 — feat: implement IPredecessorFindable interface for Repository class Implementation is correct and clean — delegates to 1. Should In oras-go, IRepository repo = registry.GetRepositoryAsync("myrepo");
// Cannot call repo.GetPredecessorsAsync() — must cast:
var graphRepo = (IReadOnlyGraphStorage)repo;This breaks interface-based programming and makes DI harder. The XML Consider adding 2. nit: Redundant count assertion In Assert.Equal(expectedReferrersList.Count, predecessorList.Count);
Assert.Equivalent(expectedReferrersList, predecessorList);
3. nit: All 4 tests use 4. nit: Test method name split across lines public async Task
Repository_GetPredecessorsAsync_FallbackToTagSchema()This is harder to scan than a single line. Consider keeping method declarations on one line, even if it exceeds the 100-column soft target — the 120-column hard limit accommodates this: public async Task Repository_GetPredecessorsAsync_FallbackToTagSchema() |
|
This is a general question: I'm curious, how do we approach adding new support in this SDK? Is the assumption that the .NET SDK mirrors the APIs AND implementation approaches of oras-go? Or do we intend only for the API surface to be the same? Or is equivalency not a goal at all? The reason I ask is that there things that make sense from a go perspective that don't translate well to .NET. For example, go is by default duck-typed while C# has explicit binding to interface types from concrete classes. Also Dependency Injection is a big part of consuming .NET SDKs and this doesn't exist really in golang (natively). I am new to this project so I am genuinely curious what the convention is? Any pointers would be great! thank you. |
This is a dotnet SDK after all, so I think goal should be developing a working SDK for dotnet end users, regardless of the golang SDK implementation. We only take Golang SDK for functional reference. The issue is that I am not very experienced in dotnet development and this project has few experienced dotnet contributors. |
Thanks for clarifying. I recognize that dotnet has a different language pattern and standard than golang. I'll keep that in mind and help out with reviews as I can, as I have experience with dotnet. I appreciate your contributions. |
What this PR does / why we need it
This change makes
Repositoryclass implement theIPredecessorFindableinterface, and enables the class to useExtendedCopy.Which issue(s) this PR resolves / fixes
Resolves / Fixes #334
Please check the following list