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

[LG] Improve LG Options for replace null and markdown rendering #3743

Merged
merged 18 commits into from
Apr 21, 2020

Conversation

cosmicshuai
Copy link
Contributor

closes: #3646
In LG, now user can add two more options in LG file.

> !# @strict = true
> !# @replaceNull = ${path} is undefined  
> !# @lineBreakStyle = markdown
  • replaceNull:

the value ${path} is undefined will create a delegate for replacing null value in evaluating expression. ${path} represents the name of variables.

for example, in this case, hi ${user.name} will be evaluated to hi user.name is undefined

  • lineBreakStyle:

Currently, we support two mode, the normal text mode and markdown mode.

In markdown mode , the line break in multiline text will be automatically converted to two lines to create a newline.

Original Issue about markdown mode: #3364

@boydc2014
Copy link
Contributor

Also please add test cases lineBreak = markdown to make sure we handle all cases

@Danieladu
Copy link
Collaborator

Add some reference tests

@boydc2014
Copy link
Contributor

boydc2014 commented Apr 17, 2020

From the code and tests, the semantics of how options and imports interact looks murky to me. I'd prefer:

  1. Options come before imports.
  2. Options specified in a file override options in all imports and their imports.
    With this semantic if I have:
    a.lg: opt.null = 'foo' [b.lg]
    b.lg: opt.null = 'blah'
    Then when using a.lg and evaluating a template in b.lg I will get 'foo' for unknown values.
    This even works if you have this:
    a.lg: opt.null = 'foo' [b.lg] [c.lg]
    b.lg: opt.null = 'blah'
    c.lg: opt.null = 'woof' [b.lg]
    If you use a.lg, then I get 'foo' for unknown values in all templates.
    If you use c.lg alone, then I get 'woof' for unknown values in c.lg or b.lg
    If you use b.lg alone, then I get 'blah' for unknown values.
    The rule is that you look at the options in the top file and it defines them for all imports. Any other rule seems to complicated to implement and understand.

Yes, the rule is options comes before imports, but it's a little bit complicated than this, there is the order we are defining

  1. The options that comes from API call Evaluate(... opts), which has highest priority
  2. The options that defined in the LG file, comes at the second
  3. The options comes from imports comes at the end (things get a little complicated here)
    2.1 for multiple imports, the former or latter imported file's options have higher priority. I think there are both reason to make former or latter to have higher priority. Not sure which is better

Here, by priority, we are talking about individual level, not the whole Options object, i mean if a individual option is not specified at the top level, we will still pick the lower-priority version.

@cosmicshuai please make sure you add tests for all those cases.

@boydc2014
Copy link
Contributor

boydc2014 commented Apr 17, 2020

From the code and tests, the semantics of how options and imports interact looks murky to me. I'd prefer:

  1. Options come before imports.
  2. Options specified in a file override options in all imports and their imports.
    With this semantic if I have:
    a.lg: opt.null = 'foo' [b.lg]
    b.lg: opt.null = 'blah'
    Then when using a.lg and evaluating a template in b.lg I will get 'foo' for unknown values.
    This even works if you have this:
    a.lg: opt.null = 'foo' [b.lg] [c.lg]
    b.lg: opt.null = 'blah'
    c.lg: opt.null = 'woof' [b.lg]
    If you use a.lg, then I get 'foo' for unknown values in all templates.
    If you use c.lg alone, then I get 'woof' for unknown values in c.lg or b.lg
    If you use b.lg alone, then I get 'blah' for unknown values.
    The rule is that you look at the options in the top file and it defines them for all imports. Any other rule seems to complicated to implement and understand.

Yes, the rule is options comes before imports, but it's a little bit complicated than this, there is the order we are defining

  1. The options that comes from API call Evaluate(... opts), which has highest priority
  2. The options that defined in the LG file, comes at the second
  3. The options comes from imports comes at the end (things get a little complicated here)
    2.1 for multiple imports, the later imported file's options have higher priority.

Here, by priority, we are talking about individual level, not the whole Options object, i mean if a individual option is not specified at the top level, we will still pick the lower-priority version.

@cosmicshuai please make sure you add tests for all those cases.

Just to be very precise about this,

Assume a.lg imported b.lg then c.lg. And b.lg, c.lg don't import any lg files.

  • Let options passed in evaluation to be O1,
  • Let options defined originally in a.lg to be OA, and b.lg = OB, c.lg = OC.
  • Let options computed from a.lg, b.lg, c.lg to be OCA, OCB, OCC, computed means after consider all imported files' options
  • Let's also define a binary operation + on options, in which x + y, means merge y with x if some value is not defined in x (which means x has higher priority)

here is a mathematical representation to define it very clear

the result denoted as R,
R = O1 + OCA  // the result = the merge of the options in paramter and the options computed from the file
OCA = OA + (OCC + OCB) // a file's computed options is the merge of it's own defined options + imported files's computed options
OCC = OC // because c.lg don't import any other files
OCB = OB

@cosmicshuai

@cosmicshuai
Copy link
Contributor Author

cosmicshuai commented Apr 17, 2020

From the code and tests, the semantics of how options and imports interact looks murky to me. I'd prefer:

  1. Options come before imports.
  2. Options specified in a file override options in all imports and their imports.
    With this semantic if I have:
    a.lg: opt.null = 'foo' [b.lg]
    b.lg: opt.null = 'blah'
    Then when using a.lg and evaluating a template in b.lg I will get 'foo' for unknown values.
    This even works if you have this:
    a.lg: opt.null = 'foo' [b.lg] [c.lg]
    b.lg: opt.null = 'blah'
    c.lg: opt.null = 'woof' [b.lg]
    If you use a.lg, then I get 'foo' for unknown values in all templates.
    If you use c.lg alone, then I get 'woof' for unknown values in c.lg or b.lg
    If you use b.lg alone, then I get 'blah' for unknown values.
    The rule is that you look at the options in the top file and it defines them for all imports. Any other rule seems to complicated to implement and understand.

Yes, the rule is options comes before imports, but it's a little bit complicated than this, there is the order we are defining

  1. The options that comes from API call Evaluate(... opts), which has highest priority
  2. The options that defined in the LG file, comes at the second
  3. The options comes from imports comes at the end (things get a little complicated here)
    2.1 for multiple imports, the former or latter imported file's options have higher priority. I think there are both reason to make former or latter to have higher priority. Not sure which is better

Here, by priority, we are talking about individual level, not the whole Options object, i mean if a individual option is not specified at the top level, we will still pick the lower-priority version.

@cosmicshuai please make sure you add tests for all those cases.

@chrimc62 @boydc2014 @Danieladu
In my current solution, I take the EvaluationOptions passed to Templates.Evaluate method as the first chioce.
If not EvaluationOptions was given in Templates.Evaluate. The options defined in the entrance file has the highest priority. If some options' values were null in the entrance LG file, such as user do not define replaceNull in current file. We will looking for the this option available in its import LG files.
The order now I take is, at the same level, I consider the former import first. And the option for each imported file was caculated recursively.
By the notation from Dong, for example, a.lg imports b.lg and c.lg, b.lg imports d.lg, if there is no given EvaluationOptions in Templates.Evaluate,
finalOption = optA + (optB + optD) + optC
This is equivalent to:
finalOption = (optA + optB) + optD + optC
Due to the associative law (this can be derived by enumarationg all possible conditions).

@chrimc62
Copy link
Contributor

I get API trumping file, but I don't get 2.1. I think if you do anything but what I suggested it will be very confusing. I really think these are "global options" where the first file encountered while following the chain that defines them defines them for any file it imports. Otherwise it gets incredibly confusing especially if the same file is included on multiple paths. Because our LG is rooted in a single file, the rule I supplied ensures you can always control the global options in a predictable way for all imported files.


In reply to: 615052805 [](ancestors = 615052805)

@chrimc62
Copy link
Contributor

It is way better to reply to a comment than to make a new comment. That way the connection is obvious and you can resolve the whole thing all at once.

@chrimc62
Copy link
Contributor

Is there a single evaluation options used for all evaluates of a given root template? If so, I don't think anything other than API and then top-level options make sense. If I do not define a null option and I import some file, I am not expecting all references in other templates to use that null option. If we rely only on the top-level file then we ignore what is in imported files and I think that is the right model in this case.

On the other hand if evaluation options are used per-file and different files can have different options, then I think "parent" import files override whatever is in "child" import files. It should never be the case that child import files provide options for parents.


In reply to: 615248909 [](ancestors = 615248909)

@boydc2014
Copy link
Contributor

Is there a single evaluation options used for all evaluates of a given root template? If so, I don't think anything other than API and then top-level options make sense. If I do not define a null option and I import some file, I am not expecting all references in other templates to use that null option. If we rely only on the top-level file then we ignore what is in imported files and I think that is the right model in this case.

On the other hand if evaluation options are used per-file and different files can have different options, then I think "parent" import files override whatever is in "child" import files. It should never be the case that child import files provide options for parents.

In reply to: 615248909 [](ancestors = 615248909)

I see, i think i get your proposal now, when you saying top file override the options of imports, you were not saying "it just has a high priority, if if there is not a value here, let the imports comes up one", you are saying, we don't do any inherit from imports, right?

That do looks simple and clean. We were dealing with is the order of inheriting options from imported files, like this.

> there are partial options defined in root file
>! @optionA = ..
[import] (a.lg)   // there are also partial options defined here
[import] (b.lg)   // there are also partial options defined here

OK, i agree, it would be very simple and easy to reason if we simply don't consider the options in imported files. We only consider what the API passed in, and the options defined in root file, right?

Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LG options improvements
5 participants