Locale awareness in Adaptive-Expressions #4276
Conversation
| I generally like the changes. Is there a work item to update all of the adaptive expression documentation for the new parameters? #Resolved |
The workflow is for us, developers make sure we have everything for doc in code as comments, and Emily will update the docs in bots-doc repo, @cosmicshuai you need to explicit about the parameters in comments. #Resolved |
| @chrimc62 @boydc2014 |
| object value = null; | ||
| string error = null; | ||
| IReadOnlyList<object> args; | ||
| var locale = options.Locale != null ? new CultureInfo(options.Locale) : Thread.CurrentThread.CurrentCulture; |
There was a problem hiding this comment.
nit, in general I would call the variable and parameter cultureInfo rather than locale so it doesn't get confused with the locale string we use in the other places.
There was a problem hiding this comment.
Agree. One reason I choose locale is I want to use same property name in both C# and JS.
Will discuss with Dong to choose a more accurate pamameter name here.
I think we should just break compatibility. I doubt anyone is actually using these functions in custom functions and if they are they are a sophisticated developer. Refers to: libraries/AdaptiveExpressions/FunctionUtils.cs:214 in 7d33aac. [](commit_id = 7d33aac, deletion_comment = False) |
| Looks good to me and @chrimc62 now, can @mrivera-ms or @gabog help take a second look on this? |
| @carlosscastro can you help review this? |
| @gabog @johnataylor |
| public static string GetLocale(this DialogContext dialogContext) | ||
| { | ||
| var locale = (dialogContext.Context as TurnContext).Locale ?? null; | ||
| return locale; |
There was a problem hiding this comment.
Avoid soft casts, it will return null if Context is not of the expected type and you will get a null ref exception.
I would refactor this line of code as:
return ((TurnContext)dialogContext.Context).Locale
But see my note about adding a new Locale property in TurnContext
If you decide to keep this method add unit tests for it.
There was a problem hiding this comment.
I think this is followed by the previous comment Tom gave, and sure @cosmicshuai we should add UT for this.
There was a problem hiding this comment.
removed the soft cast here.
There was a problem hiding this comment.
I didn't find an appropriate place to add a unit test for this. Do you have any suggestion for this? @gabog
But I have an integration test for this logic in Adaptive-Templates-Tests.
There was a problem hiding this comment.
The convention is to put unit tests un a file that has the same name of the class being tested + test. If we don't have, we should create one.
Assuming you moved the code into dialogcontext, the test should be in a file called DialogContextTests.cs
There was a problem hiding this comment.
Got it. I will create a new PR to add test for this.
This PR contains the work in #4025. Since some force-pushed commits in previous PR introduced too many stuffs in the timeline, I created this new PR and closed that one.
@tomlm @chrimc62 @cleemullins @boydc2014 @Danieladu
Locale is obtained from 'turn.activity.locale' when an incoming activity is recieved and the locale info will be set to 'turn.locale' in memory. Also user can use TurnContext.Locale to get and set locale.
After a locale is set in TurnContext. It will be passed to EvaluationOptions in TemplateEngineLanguageGenerator. Then Options in Adaptive-Expressions will be updated.
In locale related pre-built functions, the priority of locale follows: locale in APIs > locale from TurnContext > Thread.CurrentThread.CurrentCulture
Functions support locale paramter:
string(Object input, string locale?)
formatNumber(number input, integer precision, string locale?)
addToTime(string timestamp, integer interval, string unit, string format?, string locale?)
startOfDay(string timestamp, string format?, string locale?)
startOfHour(string timestamp, string format?, string locale?)
startOfMonth(string timestamp, string format?, string locale?)
convertToUTC(string timestamp, string timezone, string format?, string locale?)
convertFromUTC(string timestamp, string timezone, string format?, string locale?)
utcNow(string format?, string locale?)
getPastTime(integer interval, string unit, string format?, string locale?)
getFutureTime(integer interval, string unit, string format?, string locale?)
subtractFromTime(string timestamp, integer interval, string unit, string format?, string locale?)
formatEpoch(long epoch, string format?, string locale?)
formatTicks(long tictks, string format?, string locale?)
formatDateTime(string timestamp, string format?, string locale?)
addDays(string timestamp, int interval, string format?, string locale?)
addHours(string timestamp, int interval, string format?, string locale?)
addMinutes(string timestamp, int interval, string format?, string locale?)
toUpper(string input, string locale?)
toLower(string input, string locale?)
sentenceCase(string input, string locale?)
titleCase(string input, string locale?)
For all of these functions, locale parameter is optional. An locale paramter should be a valid locale string, such as "en-US", "en-GB", "fr-FR", "de-DE", "zh-CN", "ru-RU" etc.