Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the .NET translation services by hoisting duplicated streaming/result/error-handling helpers into BaseTranslationService, so Gemini/OpenAI-compatible/Doubao implementations share the same logic.
Changes:
- Moved
CleanupResultandCreateErrorFromResponseintoBaseTranslationServiceand removed local copies. - Added
ConsumeStreamAsyncto collapseIAsyncEnumerable<string>into a single string and updated streaming services to use it. - Removed duplicated Gemini prompt/language lists by reusing
BaseOpenAIServiceshared constants/language list; simplified Doubao post-processing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dotnet/src/Easydict.TranslationService/Services/BaseTranslationService.cs | Adds shared helpers: result cleanup, HTTP error parsing, and async stream consumption. |
| dotnet/src/Easydict.TranslationService/Services/BaseOpenAIService.cs | Uses shared stream consumer; changes shared language list/prompt visibility for reuse. |
| dotnet/src/Easydict.TranslationService/Services/GeminiService.cs | Removes duplicated prompt/languages and local helpers; uses shared helpers/constants. |
| dotnet/src/Easydict.TranslationService/Services/DoubaoService.cs | Replaces chunk accumulation + quote removal helper with shared stream consumer + cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected static async Task<string> ConsumeStreamAsync( | ||
| IAsyncEnumerable<string> stream, CancellationToken cancellationToken) | ||
| { | ||
| var sb = new StringBuilder(); | ||
| await foreach (var chunk in stream.WithCancellation(cancellationToken)) | ||
| { |
There was a problem hiding this comment.
ConsumeStreamAsync calls stream.WithCancellation(cancellationToken), but this file doesn’t import System.Runtime.CompilerServices (where the WithCancellation extension lives). Unless there’s a project-wide global using for that namespace, this won’t compile. Add the missing using, or avoid WithCancellation by manually iterating via GetAsyncEnumerator(cancellationToken).
| /// Common set of languages supported by most LLM services. | ||
| /// </summary> | ||
| protected static readonly IReadOnlyList<Language> OpenAILanguages = new[] | ||
| public static readonly IReadOnlyList<Language> OpenAILanguages = new[] |
There was a problem hiding this comment.
OpenAILanguages was changed from protected to public only so GeminiService can reference it. Since GeminiService is in the same assembly, internal would satisfy that requirement without expanding the library’s public API surface (which is harder to change later). Consider making this internal (and, if needed, exposing it through a more intentional API).
| public static readonly IReadOnlyList<Language> OpenAILanguages = new[] | |
| internal static readonly IReadOnlyList<Language> OpenAILanguages = new[] |
| /// Instructs the model to act as a translation expert. | ||
| /// </summary> | ||
| protected const string TranslationSystemPrompt = """ | ||
| public const string TranslationSystemPrompt = """ |
There was a problem hiding this comment.
TranslationSystemPrompt was changed from protected to public to enable cross-service reuse. Because the consumer (GeminiService) is in the same assembly, internal would avoid widening the public API surface while still enabling sharing. Consider switching this to internal (or moving the shared prompt to a dedicated internal/shared constants location).
| public const string TranslationSystemPrompt = """ | |
| internal const string TranslationSystemPrompt = """ |
| if (doc.RootElement.TryGetProperty("error", out var error)) | ||
| { | ||
| if (error.TryGetProperty("message", out var msgElement)) | ||
| { | ||
| message = msgElement.GetString() ?? message; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (doc.RootElement.TryGetProperty("error", out var error)) | |
| { | |
| if (error.TryGetProperty("message", out var msgElement)) | |
| { | |
| message = msgElement.GetString() ?? message; | |
| } | |
| if (doc.RootElement.TryGetProperty("error", out var error) && | |
| error.TryGetProperty("message", out var msgElement)) | |
| { | |
| message = msgElement.GetString() ?? message; |
…hods to BaseTranslationService Move CleanupResult, CreateErrorFromResponse, and ConsumeStreamAsync to BaseTranslationService so GeminiService, BaseOpenAIService, and DoubaoService share them instead of maintaining identical copies. - CleanupResult: promoted to protected static in BaseTranslationService; removed from BaseOpenAIService and GeminiService - CreateErrorFromResponse: promoted to protected in BaseTranslationService; removed from BaseOpenAIService and GeminiService - ConsumeStreamAsync: new protected static helper that consumes IAsyncEnumerable<string> into a single string; simplifies TranslateInternalAsync in all three streaming services - OpenAILanguages / TranslationSystemPrompt: changed from protected to public in BaseOpenAIService so GeminiService can reference them directly; removed duplicate _geminiLanguages list and TranslationSystemPrompt const - DoubaoService: replaced List<string> + RemoveSurroundingQuotes with ConsumeStreamAsync + CleanupResult; single-quote removal kept as Doubao-specific post-processing - Removed dead code: unused StringBuilder in GeminiService.ParseGeminiStreamAsync https://claude.ai/code/session_01VQsNADY1W8m6MzYWwc4SNo
- Add missing `using System.Runtime.CompilerServices` for WithCancellation - Narrow OpenAILanguages and TranslationSystemPrompt from public to internal (same-assembly access is sufficient; InternalsVisibleTo covers tests) - Combine nested if statements in CreateErrorFromResponse https://claude.ai/code/session_01VQsNADY1W8m6MzYWwc4SNo
f6b3d1b to
b8e5adc
Compare
Move CleanupResult, CreateErrorFromResponse, and ConsumeStreamAsync to
BaseTranslationService so GeminiService, BaseOpenAIService, and DoubaoService
share them instead of maintaining identical copies.
removed from BaseOpenAIService and GeminiService
removed from BaseOpenAIService and GeminiService
IAsyncEnumerable into a single string; simplifies
TranslateInternalAsync in all three streaming services
public in BaseOpenAIService so GeminiService can reference them directly;
removed duplicate _geminiLanguages list and TranslationSystemPrompt const
ConsumeStreamAsync + CleanupResult; single-quote removal kept as
Doubao-specific post-processing
https://claude.ai/code/session_01VQsNADY1W8m6MzYWwc4SNo