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
872 stars 480 forks source link

[DCR] LG API Revisit #3084

Closed boydc2014 closed 4 years ago

boydc2014 commented 4 years ago

Background

As LG package moving steadily towards GA, we've gained many usage from different scenarios like

Those are different scenarios focusing on slightly different parts of LG features. Some are more focusing on evaluation, others may focus on validation or authoring. We did support all those scenarios well, and in each scenario we also support multiple files and multi-language.

Along the process of supporting more and more new scenarios, we created several public facing API around TemplateEngine, StaticChecker, LGParser, LGResource. Those APIs and concepts are added progressively, and not designed or reviewed consistently under the same context or same criteria.

Some APIs are powerful and flexible enough to use with different usage patterns, in which some patterns are not what we designed for, or what we should guide user to, especially in the multiple file, multiple language scenario[See later for more explanation]. Some API are not convenient enough to result in a nice and clean caller side code, which is kind of important as a library.

Now it's a good timing to visit those API and concept, to make sure we delivered a few high cohesion low coupling interface that covers our scenarios well, and covers them in an elegant approach.

Issues

1. Unify multi-file support

Context

This issue raised my attention when we are comparing the two different approach to support multi-language fallback, the approach in master today and the approach we did in our samples to support waterfall dialog.

The difference isn't really about whether we have language-fallback in import syntax or not, the key difference here is

The code in master starts from a lg file (or "id" to be specific), then route based on locale. In this way, multiple file is supported via and only via file reference. This should the pattern we want to suggest and guide our user to. The code in sample starts from locale and then don't require any explicit lg file id.

The later approach don't align with the usual pattern that, create an engine from a file, then start using. The reason that the latter approach work, is we support the AddFiles api, which offered another way to associate multiple files, besides the import syntax inside file.

Suggestion

We should remove AddFiles api, and let TemplateEngine (or whatever new name) to explicit start from certain file. By doing that, we don't loose any capability, and we can enforce the pattern to be LG starts from a certain file(id). Which is the pattern of Adaptive, and any other programming languages.

2. Simplify + unify the concept and interface

Context

As said above, LG is now used in different scenarios, requiring different feature sets. There are 4 core feature LG provided.

As we can see above, different features are provided by different isolated components, if you want to evaluate something, you should use TemplateEngine, if you want to check without evaluation, you will choose 'StaticChecker', for editing experience like Composer, you will use LGParser to get a LGResource. Those features are essentially just different aspects around one core concept of "LGFile", and should not be broken into parts causing confusing and inconvenience to users.

Besides the features are provided separately, another core issue is the inconsistency across those feature providers, especially around error handling. CRUD is by designed to be "best effort and fault tolerant", which don't throws exceptions, but the underlying parser it depends on will throw exception on syntax errors. This caused a lot of extra efforts to mitigate on Composer side.

Suggestion

The solution is nearly out that we should consolidate the interface to have minimal concepts, and keep the related features close and consistent.

So i proposed we follow the convention in expression that, everything starts with a parser, and the parser generated an object that holds everything user would care about (evaluation, validation, internal structure and CRUD).

To be specific, two major interface LGParser and LGFile

// LGParser is the entry point and don't support `AddFiles`
class LGParser {
     LGFile ParseFile(string filePath, ImportResolver resolver);
     LGFile ParseContent(string content, string id, ImportResolver resolver);
} 

class LGFile {
    // Evalution
    object EvaluateTemplate(string templateName, object memory);
    object Evaluate(string inlineText, object memory);

    // Structure
    List<LGTemplate> templates;
    List<LGImports> imports;
    List<LGFile> references; 
    string content;

    // Validation
    List<Diagnostic> diagnostics;

    // CRUD
    // ... AddTemplate;
    // ... UpdateTemplate;
    // ... DeleteTemplate;

    // Analyzing
    AnalyzerResult AnalyzeTemplate(string tempalteName)
}

A few key points

BTW, ExpressionEngine should be also renamed to ExpressionParser, we already see the definition like this in dialog that

ExpressionEngine expressionParser

3 Hide Evaluate method

Evaluate is a method that evaluate the input by generating an anonymous template to evaluate, which is very flexible and powerful for AdaptiveDialog, but also confusing somewhat for normal user don't know about such requirement, we should seek a way to make this method an extension maybe.

xieofxie commented 4 years ago

To be honest, I once thought LG should be something independent like AdaptiveCards instead of a package in bot framework..

Danieladu commented 4 years ago

So, in this draft, LGResource class would be deprecated and LGFile will be the main entrance of the entities and functions, is the reference property value generated by the imports property? personal think a LGResource could be used to replace the templates/imports/content, these are original info. If we provide the templates, developers are confused what does it mean, means all templates including those import LG file's templates, or just the own templates this LGFile contains.

Danieladu commented 4 years ago

if i want to get all the template this LGfile has, I must foreach the references LGFiles, and merge the templates belong to them, and then merge the templates this LGFile contains itself.

Danieladu commented 4 years ago

for function object Evaluate(string inlineText, object memory);, Originally, we insert an anonymous template into the current LGFile content, and run staticchecker, then evaluate it with EvaluateTemplate. So, what is the flow of Evaluate currently, generate new content first, then use LGParser parse it again to get another new LGFile, and use the evaluateTemplate to get the result?

feich-ms commented 4 years ago

Maybe imports is not needed if we already have references

boydc2014 commented 4 years ago

@Danieladu, @feich-ms to answer you question

  1. LGFile is a sort of combination of LGResource and TemplateEngine (because LGFile support evaluation).
  2. References is directly generated by imports, because this references represents all references from it's direct imports and it's in-direct imports, it's generated by the "ResourceDiscovery" phase @feich-ms , so that we will still keep imports.
  3. We don't have to go over all references to get templates, what i listed are public interfaces, in implemenation, when we discover all references, we can keep a private member like "allTemplates", to keep track all the tempaltes discovered now, the same as the Tempaltes in templateEngine class today.
  4. Nothing changed in evaluation, like said in 3#, we will still have the allTemplates here.