-
Notifications
You must be signed in to change notification settings - Fork 281
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
Choice prompt null locale fix [Cherry-pick against 4.6] #1411
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,10 +83,7 @@ export class ChoicePrompt extends Prompt<FoundChoice> { | |
|
||
protected async onPrompt(context: TurnContext, state: any, options: PromptOptions, isRetry: boolean): Promise<void> { | ||
// Determine locale | ||
let locale: string = PromptCultureModels.mapToNearestLanguage(context.activity.locale || this.defaultLocale); | ||
if (!locale || !this.choiceDefaults.hasOwnProperty(locale)) { | ||
locale = 'en-us'; | ||
} | ||
const locale = this.determineCulture(context.activity); | ||
|
||
// Format prompt to send | ||
let prompt: Partial<Activity>; | ||
|
@@ -111,7 +108,7 @@ export class ChoicePrompt extends Prompt<FoundChoice> { | |
const utterance: string = activity.text; | ||
const choices: any[] = (this.style === ListStyle.suggestedAction ? ChoiceFactory.toChoices(options.choices) : options.choices)|| []; | ||
const opt: FindChoicesOptions = this.recognizerOptions || {}; | ||
opt.locale = activity.locale || opt.locale || this.defaultLocale || 'en-us'; | ||
opt.locale = this.determineCulture(activity, opt); | ||
const results: any[] = recognizeChoices(utterance, choices, opt); | ||
if (Array.isArray(results) && results.length > 0) { | ||
result.succeeded = true; | ||
|
@@ -120,4 +117,15 @@ export class ChoicePrompt extends Prompt<FoundChoice> { | |
|
||
return result; | ||
} | ||
|
||
private determineCulture(activity: Activity, opt: FindChoicesOptions = null): string { | ||
const optLocale = opt && opt.locale ? opt.locale : null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, prefer undefined |
||
let culture = PromptCultureModels.mapToNearestLanguage(activity.locale || optLocale || this.defaultLocale || PromptCultureModels.English.locale); | ||
if (!culture || !this.choiceDefaults[culture]) | ||
{ | ||
culture = PromptCultureModels.English.locale; | ||
} | ||
|
||
return culture; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,12 +115,35 @@ export class PromptCultureModels { | |
noInLanguage: 'No', | ||
} | ||
|
||
private static getSupportedCultureCodes(): string[] { | ||
return this.getSupportedCultures().map((c): string => c.locale); | ||
} | ||
|
||
/** | ||
* Use Recognizers-Text to normalize various potential Locale strings to a standard. | ||
* @remarks This is mostly a copy/paste from https://github.com/microsoft/Recognizers-Text/blob/master/JavaScript/packages/recognizers-text/src/culture.ts#L39 | ||
* This doesn't directly use Recognizers-Text's MapToNearestLanguage because if they add language support before we do, it will break our prompts. | ||
* @param cultureCode Represents locale. Examples: "en-US, en-us, EN". | ||
* @returns Normalized locale. | ||
*/ | ||
public static mapToNearestLanguage = (cultureCode: string): string => Culture.mapToNearestLanguage(cultureCode); | ||
public static mapToNearestLanguage(cultureCode: string): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mean cultureCode is optional now? I'd expect, given the function signature, that we'd throw an exception if someone asked for the nearestLanguage and they passed undefined. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If used directly, yes, it's optional and returns |
||
if (cultureCode !== undefined) { | ||
cultureCode = cultureCode.toLowerCase(); | ||
let supportedCultureCodes = this.getSupportedCultureCodes(); | ||
|
||
if (supportedCultureCodes.indexOf(cultureCode) < 0) { | ||
let culturePrefix = cultureCode.split('-')[0].trim(); | ||
|
||
supportedCultureCodes.forEach(function(supportedCultureCode): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer anonymous functions |
||
if (supportedCultureCode.startsWith(culturePrefix)) { | ||
cultureCode = supportedCultureCode; | ||
} | ||
}); | ||
} | ||
} | ||
|
||
return cultureCode; | ||
} | ||
|
||
public static getSupportedCultures = (): PromptCultureModel[] => | ||
[ | ||
|
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'd recommend preferring
undefined
tonull
in this case.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.
https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#null-and-undefined