Add Combined Translation Service

[FabrizioBrancati]

One way to get better translations is to ask Google Translate to do it first and then pass its result to OpenAI.

This PR adds a new CombinedTranslator service, which does this.
This requires 2 API Keys in input, of course. So the input parameter api-key (or k) is now an array.

Google Translate API Key is much shorter than the OpenAI one, so the order in the array doesn't matter, the tool can automatically understand which one to use for what, thanks to let apiToken = apiToken.sorted { $0.count < $1.count }.

[andrewtheis]

I wonder if it makes sense to surface to the user whether a consensus was reached (ie, both services ended up at the same result)?

[FabrizioBrancati]

I wonder if it makes sense to surface to the user whether a consensus was reached (ie, both services ended up at the same result)?

I think there would be too many messages on the command line. As we print every translation, the user must scroll through all the output to know when both services agreed on the translation.

Also, what's the value of doing it? Evaluate if it makes sense to use both or only one?

[andrewtheis]

/gemini

testing

[gemini-code-assist]

It looks like you were testing the /gemini command. To use it, you need to specify a command like /gemini review or /gemini summary. If you want to see the available commands, you can use /gemini help.

[andrewtheis]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a CombinedTranslator service that leverages both Google Translate and OpenAI for improved translations. The approach of using Google Translate as a first pass before passing the result to OpenAI is interesting. The code is generally well-structured, but there are a few areas that could be improved for clarity and robustness.

Summary of Findings

• Error Handling for API Key Retrieval: The code uses apiToken.first to retrieve API keys for Google and OpenAI. While this works, it could be more explicit in handling cases where the array might be empty or not contain the expected number of keys. Consider adding more descriptive error messages or using a safer method to access the keys.
• Clarity of API Key Sorting Logic: The code sorts API keys based on their length to differentiate between Google and OpenAI keys. While this works based on the assumption that Google API keys are shorter, it's not very robust and could fail if the keys have similar lengths. Consider using a more explicit method to identify the keys, such as requiring them to be passed with specific flags or using a configuration file.
• Missing Unit Tests: There are no unit tests included in this pull request. Unit tests are essential to ensure the correctness and reliability of the new CombinedTranslator service. Consider adding unit tests to cover different scenarios, such as valid and invalid API keys, different input strings, and error conditions.

Merge Readiness

The pull request introduces a useful feature, but there are some concerns regarding error handling, API key identification, and the lack of unit tests. I recommend addressing these issues before merging. I am unable to directly approve this pull request, and other reviewers should review and approve this code before merging.

[gemini-code-assist]

Consider adding a more descriptive help message that explains the expected format and order of the API keys when using the combined service. This will help users understand how to provide the correct keys. For example, you could specify that the first key should be the Google Translate API key and the second key should be the OpenAI API key.

help: "OpenAI or Google Cloud Translate (v2) API key. When using 'combined' service, provide Google Translate key first, followed by OpenAI key."

[gemini-code-assist]

Consider providing a more specific error message that indicates which API key is missing (Google Cloud Translate). This will help users troubleshoot the issue more easily.

throw ValidationError("Google Cloud Translate API key is required. Please provide a valid Google Cloud Translate API key.")

[gemini-code-assist]

Consider providing a more specific error message that indicates which API key is missing (OpenAI). This will help users troubleshoot the issue more easily.

throw ValidationError("OpenAI API key is required. Please provide a valid OpenAI API key.")

[gemini-code-assist]

Consider providing a more descriptive error message that clarifies the expected order of the API keys. This will help users understand how to provide the correct keys for the combined translation service.

throw ValidationError("Two API keys are required for combined translation: Google Translate API key followed by OpenAI API key.")

[gemini-code-assist]

The logic for sorting API keys based on length is not very robust. It assumes that Google API keys are always shorter than OpenAI keys, which might not always be the case. Consider using a more explicit method to identify the keys, such as requiring them to be passed with specific flags or using a configuration file. If you keep this approach, consider adding a comment explaining why the keys are sorted by length.

// Sort API keys: Assuming Google API key is shorter than OpenAI key
let apiToken = apiToken.sorted { $0.count < $1.count }
let googleAPIToken = String(apiToken[0])
let openAIAPIToken = String(apiToken[1])

[gemini-code-assist]

Consider adding error handling to catch any exceptions thrown by the googleTranslator or openAITranslator. This will make the code more robust and prevent unexpected crashes.

do {
    let googleTranslation = try await googleTranslator.translate(string, to: targetLanguage, comment: comment, baseTranslation: nil)
    return try await openAITranslator.translate(string, to: targetLanguage, comment: nil, baseTranslation: googleTranslation)
} catch {
    print("Error during combined translation: \(error)")
    throw error // Re-throw the error to be handled upstream
}

[gemini-code-assist]

The comment parameter in chatQuery has a default value of nil, which is good. However, the baseTranslation parameter also has a default value of nil, which might not be ideal. Consider making the baseTranslation parameter required, as it's essential for the CombinedTranslator service. If you keep the default value, consider adding a comment explaining why it's optional.

Suggested change

	private func chatQuery(for translatableText: String, targetLanguage: Language, comment: String? = nil, baseTranslation: String? = nil) -> ChatQuery {
	private func chatQuery(for translatableText: String, targetLanguage: Language, comment: String? = nil, baseTranslation: String) -> ChatQuery {