microsoft / botbuilder-dotnet

Welcome to the Bot Framework SDK for .NET repository, which is the home for the libraries and packages that enable developers to build sophisticated bot applications using .NET.
https://github.com/Microsoft/botframework
MIT License
873 stars 484 forks source link

use lg instead of expression in value, condition etc. #3652

Closed xieofxie closed 4 years ago

xieofxie commented 4 years ago

Use this query to search for the most popular feature requests.

Is your feature request related to a problem? Please describe. I'm always frustrated when I have to write long expressions and copy-paste common expressions.

For example, convert a set/map to list with index: =select(indicesAndValues(user.taskLists[dialog.listType]),iter,setProperty(setProperty(setProperty({}, "Id", iter.index),"Topic",iter.value.content),"IsCompleted",iter.value.checked))

It is very long and the conversion function (data structure to structure with id) has to be copy-paste everywhere

Describe the solution you'd like Use lg instead of expression to unblock the following scenarios: Reuse common functions

# Convert(index, value)
- ${setProperty(setProperty(setProperty({}, "Id", index),"Topic",value.content),"IsCompleted",value.checked)}
# GetList(listType)
- ${indicesAndValues(user.taskLists[listType])}

Shorten expressions =select(GetList(dialog.listType),iter,Convert(iter.index,iter.value))

Describe alternatives you've considered

Additional context Common happened in composer

[enhancement]

boydc2014 commented 4 years ago

I think I've seen such an issue somewhere before in SDK repo, that a user is asking for using LG to customize functions, and also seen user in Composer side asking similar thing.

I don't know about this, feels weird to enable LG to do this, given the scope of LG is "language generating", but at other hand, deal with long or customized expression or reuse expression is easier in LG, @vishwacsena @tomlm any thoughts

hibrenda commented 4 years ago

https://github.com/microsoft/BotFramework-Composer/issues/2413

vishwacsena commented 4 years ago

@boydc2014 @tomlm I do not believe we need to set this constraint on the user. In a tool like composer, the customer will look at a single cohesive experience and not necessarily want to (or should) understand the distinction between LG templates being used only in specific parts of their dialog.

Do you see strong reasons why we should not enable this?

My take: We should allow injecting all templates as functions so it is usable from dialog as well. we should document that in cases where a template name conflicts with a builtin function, users must use ${lg.templateName()} to uniquely resolve template added via LG.

This does not preclude customers from directly injecting additional functions via the bot runtime and having it exposed throughout composer. However for that workflow, we still need to figure out how to enable auto-suggest, auto-complete experience in composer. With the LG approach the customers are looking for, you'd get that for free/ existing mechanisms.

vishwacsena commented 4 years ago

Thanks to @cwhitten, here's another example of customer confusion https://stackoverflow.com/questions/60752270/why-condition-field-only-accepts-pre-built-functions

chrimc62 commented 4 years ago

An alternative would be to add to adaptive expressions the ability to define custom functions in terms of other expressions. Something like define("foo(a, b, c)", a + b + c) which would build an evaluator for "foo" that would take 3 args a, b, c and is defined in terms of other expressions and define that as a custom function. The problem with using .lg is that it mixes the layers and you have for example two ways to do if, i.e. IF: or the if function. The next question would be how to expose this. Could be a custom functions property on Adaptive dialog for example that has an array of expressions to execute--if they are defines they add to the custom functions and otherwise just get executed. Our functions would have no side-effects except for define, but custom functions could. I don't think we need types or variable arity--they are already handled by the leaf functions and we can keep this simple.
Alternatively we could have a new $kind Microsoft.CustomFunction which has a name like "foo(a, b, c)" and an expression that would add to the custom function table when loaded.

vishwacsena commented 4 years ago

@chrimc62 Isn't both dialog and LG use a common expression engine instance? If not true, we should fix that because without it users cannot register additional functions to expressions that is available in both dialog and LG.

Now with dialog and LG sharing the same expression instance, the real question here is if LG should add the templates to the function table. I'd say it should for all the reasons I had called out above.

Based on your input above, it is unclear to me on what your actual proposal is. I hope we do not have to have another way to do prebuilt functions for dialogs..?

chrimc62 commented 4 years ago

@tomlm any comments?

vishwacsena commented 4 years ago

@tomlm and I chatted and came up with a following scope reduction and clarification. The main goal for us to enable here is for customers to be able to inject specific functions to the global function table to be usable within dialog. We do not want to auto-inject all LG templates because

While we enable this, we should set users up for success so they do not accidentally overwrite the default set of prebuilt functions in the function table. At the moment each template engine instance has its own function table where the LG functions are injected but we would like to give users a way to inject into the global function table that is also used by the dialog sub-system.

using praser instructions to specify this

> namespace is required. if missing, use the file name (or diaog name as the namespace).
> a single LG file can only have one namespace. Multiple LG files can have the same namespace definition.
> !# @namspace = foo
> Add these templates to global function table, with the namespace prefix. The only way user can consume these are via ${namespace.templateName()} notation from within dialog. 
> the value to this is a comma separated list of one or more templateNames that are defined in this .lg file.
> !# @addToGlobalFunctionTable = templateName1, templateName2, ...

# templateName1() 
- <template definition>
vishwacsena commented 4 years ago

@boydc2014 any concerns? Can we do this in a clean way without any impact to non adaptive consumption of LG as well?

boydc2014 commented 4 years ago

@boydc2014 any concerns? Can we do this in a clean way without any impact to non adaptive consumption of LG as well?

No concern of impacting non-adaptive consumption. Perhaps we can unify the options naming a little bit like

> !# @registerNamespace = foo
> !# @registerAsFunctions = template1, template2
boydc2014 commented 4 years ago

After chatting with @vishwacsena and @cwhitten about this, this design need some refinement on how multi-language lg files can be handled.

It's unclear to me and do we how to create a "abstract function" on top of every language, which could very complicated, when dealing with how to get and pass in locale. Also have all injection statement in root.lg don't seems to be right, because from a completeness POV, the root.lg haven't seen those templates yet.

cleemullins commented 4 years ago

added @vishwacasena per @tomlm

cleemullins commented 4 years ago

This should land this week, per @vishwacsena and @boydc2014

boydc2014 commented 4 years ago

This should land this week, per @vishwacsena and @boydc2014

yes, we synced about this, and simplified the design, it should be at least a PR this week ready for review.

cleemullins commented 4 years ago

@boydc2014, 7 days ago you said "PR this week". Can you update this issue?