refactor: 档案重做#3061
Draft
LinQingYuu wants to merge 6 commits into
Draft
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
审查者指南（Reviewer's Guide）引入了一个新的、基于 IdentityModel 的 Minecraft 档案（profile）子系统脚手架，包括档案存储抽象、旧/新档案模型，以及 Xbox/Microsoft 身份验证辅助类型，并增加了一个由生命周期管理的 ProfileService 占位组件，用于未来的迁移/导入逻辑。 文件级变更
与关联 Issue 的符合度评估
可能关联的 Issue
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
Original review guide in EnglishReviewer's GuideIntroduces a new, identity-model-based Minecraft profile subsystem scaffold, including profile storage abstractions, old/new profile models, and Xbox/Microsoft authentication helper types, plus a lifecycle-managed ProfileService placeholder for future migration/import logic. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e42b1fb48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,34 @@ | |||
| namespace PCL.Core.Minecraft.Profile; | |||
|
|
|||
| public class ProfileManagement<T>: IProfileManagement<T> where T:class | |||
There was a problem hiding this comment.
Because PCL.Core.csproj explicitly compiles **\*.cs, this nested copy is built together with the new files directly under PCL.Core/Minecraft/Profile/. That creates duplicate PCL.Core.Minecraft.Profile.ProfileManagement<T>/IProfileManagement<T> definitions and duplicate ProfileService members in the same namespace, so PCL.Core cannot build until one copy of the profile tree is removed.
Useful? React with 👍 / 👎.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体性的反馈：
ProfileService、IProfileManagement<T>和ProfileManagement<T>在Minecraft/Profile/和Minecraft/Profile/Profile/路径下都有内容相同的重复定义；建议将每种类型整合为单个文件，以避免类型重复定义和后续维护上的困惑。- 在
_MigrateProfile中，与profileMigrateFIle相关的逻辑似乎是反的（当Path.Exists返回 false 时记录“已迁移档案信息，跳过检查”），并且profileLocation和 try 代码块目前都未使用/为空；请修正条件并实现迁移逻辑，或者移除占位代码，以避免产生误导性的行为。 - 若干新增类型和方法（例如
ProfileManagement<T>的方法、MicrosoftProvider、MojangUtils、Profile.CreateAuthenticateServiceProvider等）当前为空或直接抛出NotImplementedException；如果计划近期使用这些类型，建议至少添加最小可用的行为或清晰的 TODO 注释，以便让未来维护者明确了解其当前局限性。
提供给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- There appear to be duplicate definitions for `ProfileService`, `IProfileManagement<T>`, and `ProfileManagement<T>` under both `Minecraft/Profile/` and `Minecraft/Profile/Profile/` paths with identical content; consider consolidating to a single file per type to avoid type redefinition and future maintenance confusion.
- In `_MigrateProfile`, the logic around `profileMigrateFIle` seems inverted (logging "已迁移档案信息，跳过检查" when `Path.Exists` returns false) and both `profileLocation` and the try-block are currently unused/empty; please either correct the condition and implement the migration or remove the placeholder code to avoid misleading behavior.
- Several new types and methods (`ProfileManagement<T>` methods, `MicrosoftProvider`, `MojangUtils`, `Profile.CreateAuthenticateServiceProvider`, etc.) are currently empty or throw `NotImplementedException`; if these are intended to be used soon, consider at least adding minimal behavior or clear TODO comments so their current limitations are explicit to future maintainers.
## Individual Comments
### Comment 1
<location path="PCL.Core/Minecraft/Profile/Profile/ProfileService.cs" line_range="25-28" />
<code_context>
+ {
+
+ var profileLocation = Path.Combine(Paths.SharedData, "profiles.json");
+ var profileMigrateFIle = Path.Combine(Paths.SharedData, "pcl.ce.migrated");
+ if (!Path.Exists(profileMigrateFIle))
+ {
+ Context.Debug("已迁移档案信息，跳过检查");
+ return;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The migration guard condition appears inverted relative to the log message.
The log is emitted when `!Path.Exists(profileMigrateFIle)`, i.e. when the marker file is missing, which implies migration has not run yet. Usually you’d only skip migration when the marker file exists. Please either invert the condition or adjust the log so the marker file consistently represents “migration already done.”
</issue_to_address>
### Comment 2
<location path="PCL.Core/Minecraft/Profile/Profile/ProfileService.cs" line_range="17-12" />
<code_context>
+ private static ProfileManagement<ProfileJson<Models.OldProfile>> _oldProfileProvider = new();
+
+ [LifecycleStart]
+ private static async Task _Start()
+ {
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Avoid `async` on `_Start` since it currently does not `await` anything.
This empty `async` method will produce a compiler warning and allocate an unnecessary async state machine. Either drop `async` and return `Task.CompletedTask`, or add the actual asynchronous work so `async` is justified.
Suggested implementation:
```csharp
[LifecycleStart]
private static Task _Start()
```
```csharp
private static Task _Start()
{
return Task.CompletedTask;
}
```
If `using System.Threading.Tasks;` is not already present at the top of `ProfileService.cs`, it should be added so that `Task` and `Task.CompletedTask` resolve correctly.
</issue_to_address>
### Comment 3
<location path="PCL.Core/Minecraft/Profile/ProfileManagement.cs" line_range="3-4" />
<code_context>
+namespace PCL.Core.Minecraft.Profile;
+
+public class ProfileManagement<T>: IProfileManagement<T> where T:class
+{
+ public T Create()
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** There are duplicate `ProfileManagement<T>` definitions in the same namespace.
This class also exists in `Profile/ProfileManagement.cs` under the same `PCL.Core.Minecraft.Profile` namespace, which will cause a type redefinition compile error. Please remove one definition or change the namespace so only one `ProfileManagement<T>` exists in this namespace.
</issue_to_address>
### Comment 4
<location path="PCL.Core/Minecraft/Profile/IProfileManagement.cs" line_range="3-4" />
<code_context>
+namespace PCL.Core.Minecraft.Profile;
+
+public interface IProfileManagement<T>
+{
+ /// <summary>
+ /// 创建一个档案
</code_context>
<issue_to_address>
**issue (bug_risk):** There are duplicate `IProfileManagement<T>` interfaces in the same namespace.
`IProfileManagement<T>` is defined in both `Minecraft/Profile/IProfileManagement.cs` and `Minecraft/Profile/Profile/IProfileManagement.cs` under the same namespace with identical members, which creates a duplicate type. Please consolidate to a single definition or adjust namespaces so only one is visible to consumers.
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈不断改进审查质量。
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- There appear to be duplicate definitions for
ProfileService,IProfileManagement<T>, andProfileManagement<T>under bothMinecraft/Profile/andMinecraft/Profile/Profile/paths with identical content; consider consolidating to a single file per type to avoid type redefinition and future maintenance confusion. - In
_MigrateProfile, the logic aroundprofileMigrateFIleseems inverted (logging "已迁移档案信息，跳过检查" whenPath.Existsreturns false) and bothprofileLocationand the try-block are currently unused/empty; please either correct the condition and implement the migration or remove the placeholder code to avoid misleading behavior. - Several new types and methods (
ProfileManagement<T>methods,MicrosoftProvider,MojangUtils,Profile.CreateAuthenticateServiceProvider, etc.) are currently empty or throwNotImplementedException; if these are intended to be used soon, consider at least adding minimal behavior or clear TODO comments so their current limitations are explicit to future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There appear to be duplicate definitions for `ProfileService`, `IProfileManagement<T>`, and `ProfileManagement<T>` under both `Minecraft/Profile/` and `Minecraft/Profile/Profile/` paths with identical content; consider consolidating to a single file per type to avoid type redefinition and future maintenance confusion.
- In `_MigrateProfile`, the logic around `profileMigrateFIle` seems inverted (logging "已迁移档案信息，跳过检查" when `Path.Exists` returns false) and both `profileLocation` and the try-block are currently unused/empty; please either correct the condition and implement the migration or remove the placeholder code to avoid misleading behavior.
- Several new types and methods (`ProfileManagement<T>` methods, `MicrosoftProvider`, `MojangUtils`, `Profile.CreateAuthenticateServiceProvider`, etc.) are currently empty or throw `NotImplementedException`; if these are intended to be used soon, consider at least adding minimal behavior or clear TODO comments so their current limitations are explicit to future maintainers.
## Individual Comments
### Comment 1
<location path="PCL.Core/Minecraft/Profile/Profile/ProfileService.cs" line_range="25-28" />
<code_context>
+ {
+
+ var profileLocation = Path.Combine(Paths.SharedData, "profiles.json");
+ var profileMigrateFIle = Path.Combine(Paths.SharedData, "pcl.ce.migrated");
+ if (!Path.Exists(profileMigrateFIle))
+ {
+ Context.Debug("已迁移档案信息，跳过检查");
+ return;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The migration guard condition appears inverted relative to the log message.
The log is emitted when `!Path.Exists(profileMigrateFIle)`, i.e. when the marker file is missing, which implies migration has not run yet. Usually you’d only skip migration when the marker file exists. Please either invert the condition or adjust the log so the marker file consistently represents “migration already done.”
</issue_to_address>
### Comment 2
<location path="PCL.Core/Minecraft/Profile/Profile/ProfileService.cs" line_range="17-12" />
<code_context>
+ private static ProfileManagement<ProfileJson<Models.OldProfile>> _oldProfileProvider = new();
+
+ [LifecycleStart]
+ private static async Task _Start()
+ {
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Avoid `async` on `_Start` since it currently does not `await` anything.
This empty `async` method will produce a compiler warning and allocate an unnecessary async state machine. Either drop `async` and return `Task.CompletedTask`, or add the actual asynchronous work so `async` is justified.
Suggested implementation:
```csharp
[LifecycleStart]
private static Task _Start()
```
```csharp
private static Task _Start()
{
return Task.CompletedTask;
}
```
If `using System.Threading.Tasks;` is not already present at the top of `ProfileService.cs`, it should be added so that `Task` and `Task.CompletedTask` resolve correctly.
</issue_to_address>
### Comment 3
<location path="PCL.Core/Minecraft/Profile/ProfileManagement.cs" line_range="3-4" />
<code_context>
+namespace PCL.Core.Minecraft.Profile;
+
+public class ProfileManagement<T>: IProfileManagement<T> where T:class
+{
+ public T Create()
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** There are duplicate `ProfileManagement<T>` definitions in the same namespace.
This class also exists in `Profile/ProfileManagement.cs` under the same `PCL.Core.Minecraft.Profile` namespace, which will cause a type redefinition compile error. Please remove one definition or change the namespace so only one `ProfileManagement<T>` exists in this namespace.
</issue_to_address>
### Comment 4
<location path="PCL.Core/Minecraft/Profile/IProfileManagement.cs" line_range="3-4" />
<code_context>
+namespace PCL.Core.Minecraft.Profile;
+
+public interface IProfileManagement<T>
+{
+ /// <summary>
+ /// 创建一个档案
</code_context>
<issue_to_address>
**issue (bug_risk):** There are duplicate `IProfileManagement<T>` interfaces in the same namespace.
`IProfileManagement<T>` is defined in both `Minecraft/Profile/IProfileManagement.cs` and `Minecraft/Profile/Profile/IProfileManagement.cs` under the same namespace with identical members, which creates a duplicate type. Please consolidate to a single definition or adjust namespaces so only one is visible to consumers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+25
to
+28
| var profileMigrateFIle = Path.Combine(Paths.SharedData, "pcl.ce.migrated"); | ||
| if (!Path.Exists(profileMigrateFIle)) | ||
| { | ||
| Context.Debug("已迁移档案信息，跳过检查"); |
There was a problem hiding this comment.
issue (bug_risk): 迁移保护条件相对于日志信息来说似乎被写反了。
当前日志是在 !Path.Exists(profileMigrateFIle) 时输出的，也就是标记文件不存在时，这通常意味着迁移尚未执行。一般情况下，只有在标记文件存在时才会跳过迁移。请反转该条件，或调整日志内容，以确保该标记文件始终明确表示“迁移已完成”。
Original comment in English
issue (bug_risk): The migration guard condition appears inverted relative to the log message.
The log is emitted when !Path.Exists(profileMigrateFIle), i.e. when the marker file is missing, which implies migration has not run yet. Usually you’d only skip migration when the marker file exists. Please either invert the condition or adjust the log so the marker file consistently represents “migration already done.”
|
|
||
| [LifecycleScope("profile", "档案服务")] | ||
| public partial class ProfileService | ||
| { |
There was a problem hiding this comment.
suggestion: 避免在 _Start 上使用 async，因为当前方法内部没有任何 await 调用。
这个空的 async 方法会产生编译器警告，并且会生成一个不必要的异步状态机。可以选择去掉 async 并返回 Task.CompletedTask，或者在其中加入实际的异步逻辑，使 async 变得合理。
建议实现方式：
[LifecycleStart]
private static Task _Start() private static Task _Start()
{
return Task.CompletedTask;
}如果 ProfileService.cs 顶部尚未包含 using System.Threading.Tasks;，请添加它，以确保 Task 和 Task.CompletedTask 能正确解析。
Original comment in English
suggestion: Avoid async on _Start since it currently does not await anything.
This empty async method will produce a compiler warning and allocate an unnecessary async state machine. Either drop async and return Task.CompletedTask, or add the actual asynchronous work so async is justified.
Suggested implementation:
[LifecycleStart]
private static Task _Start() private static Task _Start()
{
return Task.CompletedTask;
}If using System.Threading.Tasks; is not already present at the top of ProfileService.cs, it should be added so that Task and Task.CompletedTask resolve correctly.
pcl-ce-automation
Bot
added
🚧 正在处理
# Conflicts: # PCL.Core/Minecraft/IdentityModel/Extensions/JsonWebToken/JsonWebKeys.cs # PCL.Core/Minecraft/IdentityModel/Extensions/JsonWebToken/JsonWebToken.cs # PCL.Core/Minecraft/IdentityModel/Extensions/OpenId/OpenIdClient.cs # PCL.Core/Minecraft/IdentityModel/Extensions/OpenId/OpenIdOptions.cs # PCL.Core/Minecraft/IdentityModel/Extensions/YggdrasilConnect/YggdrasilClient.cs # PCL.Core/Minecraft/IdentityModel/Extensions/YggdrasilConnect/YggdrasilOptions.cs # PCL.Core/Minecraft/IdentityModel/IdentityModelException.cs # PCL.Core/Minecraft/IdentityModel/Yggdrasil/Client.cs
LinQingYuu
force-pushed
the
refactor/profile
branch
from
94af9ea to
e6cb129
Compare
pcl-ce-automation
Bot
added
size: XL
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
1 participant
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.
重写档案系统，使其基于规范的 IdentityModel 而不是原先一堆问题的 ModLaunch
Resolved #17
Resolved #2867