Skip to content
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

[DCR] LG API Revisit #3084

Closed
boydc2014 opened this issue Dec 5, 2019 · 6 comments
Closed

[DCR] LG API Revisit #3084

boydc2014 opened this issue Dec 5, 2019 · 6 comments
Assignees
Labels
composer Impacts composer R8 Release 8 - March 16th, 2020
Milestone

Comments

@boydc2014
Copy link
Contributor

boydc2014 commented Dec 5, 2019

Background

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

  • Use LG standalone without even bot concept
  • Use LG with declarative SDK
  • Use LG in coding Adaptive
  • Use in legacy waterfall dialog
  • Authoring LG in Composer
  • Checking in VSCode extension
  • Analyzing in TriggerTree, FormDialog

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

  • everything starts from lg file(the code in master) or
  • everything starts from a language\locale(the code in sample)

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.

  • Evaluation.
    Provided by TemplateEngine. Required by SDK and VSCode extension.
  • Validation
    Provided by StaticChecker. Required by SDK, VSCode extension, Composer, LSP, basically every customer need this.
  • Parsing.
    Parsing means get a sense of the internal structure of a LG file (ie, how many templates are there).
    Provided by LGParser => LGResource. Required by VSCode extension and Composer.
  • CRUD(content manipulation)
    This is also provided by LGParser => LGResource. Required by Composer
  • Analyzing
    Provided by TemplateEngine via AnalyzeTempalte function. Required by Form,TriggerTree, and potential Composer.

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

  • No more engine, LGParser will generate LGFile, LGFile is where evaluation happened, not LGParse(engine).
  • LGParser will only support parsing from a file\content, no more "addText, addFiles", refering to other sources should be done using import syntax.
  • LGParser won't throw exception even on syntax errors, diagnostics will be put into diagnostics
  • LGFile will have a reference to other LGFiles, and other diagonstics in other LGFile will also prevent this LGFile to be evaluate, because 'an error in it's refernce'.

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.

@boydc2014 boydc2014 added the R8 Release 8 - March 16th, 2020 label Dec 5, 2019
@boydc2014 boydc2014 self-assigned this Dec 5, 2019
@xieofxie
Copy link
Contributor

xieofxie commented Dec 5, 2019

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

@Danieladu
Copy link
Contributor

Danieladu commented Dec 9, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

feich-ms commented Dec 10, 2019

Maybe imports is not needed if we already have references

@boydc2014
Copy link
Contributor Author

@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.

@munozemilio munozemilio added this to the R8 milestone Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composer Impacts composer R8 Release 8 - March 16th, 2020
Projects
None yet
Development

No branches or pull requests

5 participants