-
Notifications
You must be signed in to change notification settings - Fork 142
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
English to Igbo Translation Endpoint #822
Conversation
fix: send error response and proper json in ai post requests
refactor: revert changes so home page still works
Correct Translation endpoint response key
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.
Looking really good! Just a few comments about strings vs. maps
src/controllers/translation.ts
Outdated
|
||
if (igboText.length > IGBO_ENGLISH_TRANSLATION_INPUT_MAX_LENGTH) { | ||
throw new Error('Cannot translate text greater than 120 characters'); | ||
const translationKey = |
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.
Instead of casting these strings together to create a key to determine the payload shape, I think the sourceLanguageCode
could map to the desired payload shape. Something like
const PayloadKeyMap = {
[LanguageEnum.Igbo]: 'igbo',
[LanguageEnum.English]: 'english'
}
That way, we could take the value of indexing into that map to determine the final payload shape:
const payload = { [PayloadKeyMap[requestBody.sourceLanguageCode]: textToTranslate }
If requestBody.sourceLanguageCode === 'ibo'
then we would get a final payload of { igbo: textToTranslate }
(if my logic is correct 😅 )
I think there's an even cleaner way of us handling this, but I think relying on maps rather than strings to determine payload shapes is a more stable way to go
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.
Switched to something like this. Thanks for the suggestion!
src/controllers/translation.ts
Outdated
// Igbo translation input is 120 characters | ||
const IGBO_ENGLISH_TRANSLATION_INPUT_MAX_LENGTH = 120; | ||
// TODO: move this information to remote config | ||
const SUPPORTED_TRANSLATIONS = { |
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.
I think we would want to update this map to rely on nested maps for better indexing:
const SUPPORTED_TRANSLATIONS = {
[LanguageEnum.IGBO]: {
[LanguageEnum.ENGLISH]: {
maxInputLength: 120,
translationAPI: IGBO_TO_ENGLISH_API,
}
},
[LanguageEnum.ENGLISH]: {
[LanguageEnum.IGBO]: {
maxInputLength: 120,
translationAPI: ENGLISH_TO_IGBO_API,
}
}
}
So then when we use SUPPORT_TRANSLATIONS
we can index using our LanguageEnum
values:
const translation = SUPPORTED_TRANSLATIONS[requestBody.sourceLanguageCode][requestBody.destinationLanguageCode] // === { maxInputLength: 120, translationAPI: IGBO_TO_ENGLISH_API }
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.
I'm also interested to hear why we would want to move this to RemoteConfig! I think it's fine having this object here, but I might be missing something
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.
I ended up getting something to work that typescript could actually be happy with by indexing all the languages on the first level of the object. Inner languages don't need to map out each one as not to be overly exhaustive.
Describe your changes
Add English to Igbo Translation to the
/translate
endpoint.Motivation and Context
We've recently added Igbo to English translation in #812. This PR updates the code to support English to Igbo translation as well.