-
Notifications
You must be signed in to change notification settings - Fork 487
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] Normalize all expression and add '=' to force expression. #3253
Conversation
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3 |
rename FloatExpression to NumberExpression to better align with javascript (int, number) and json schema (int, number) Remove LongExpression changed to dcState pattern
libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Actions/BaseInvokeDialog.cs
Outdated
Show resolved
Hide resolved
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3 |
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3 |
|
||
if (activity.LocalTimestamp == null || activity.LocalTimestamp == default(DateTimeOffset)) | ||
{ | ||
activity.LocalTimestamp = DateTimeOffset.Now; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this fixing? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time recognizer tests would fail when run in the morning because it was executing in UTC, so tommorrow gives you a different date. TestAdapter should have been setting this.
In reply to: 367263801 [](ancestors = 367263801)
|
||
if (this.disabled != null && (bool?)this.disabled.TryEvaluate(dc.GetState()).value == true) | ||
if (this.Disabled != null && this.Disabled.GetValue(dcState) == true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.Disabled != null [](start = 16, length = 21)
This is testing to see if there is an expression assigned, right? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[JsonConstructor] | ||
public EmitEvent(string eventName = null, string eventValue = null, bool bubble = false, [CallerFilePath] string callerPath = "", [CallerLineNumber] int callerLine = 0) | ||
public EmitEvent(string eventName = null, object eventValue = null, bool bubble = false, [CallerFilePath] string callerPath = "", [CallerLineNumber] int callerLine = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object [](start = 50, length = 6)
This seems like a good chsnge. #ByDesign
@@ -167,13 +165,22 @@ public override async Task<DialogTurnResult> BeginDialogAsync(DialogContext dc, | |||
JToken instanceBody = null; | |||
if (this.Body != null) | |||
{ | |||
instanceBody = (JToken)this.Body.DeepClone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeepClone [](start = 49, length = 9)
Is FromObject better than DeepClone? #Resolved
@@ -3,6 +3,7 @@ | |||
|
|||
using System; | |||
using System.Collections.Generic; | |||
using System.ComponentModel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this? #Resolved
using Newtonsoft.Json; | ||
//using System; | ||
//using Microsoft.Bot.Expressions; | ||
//using Newtonsoft.Json; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete whole file? #Resolved
@@ -1338,10 +1330,6 @@ private static (object value, string error) Substring(Expression expression, IMe | |||
} | |||
} | |||
} | |||
else | |||
{ | |||
error = $"{expression.Children[0]} is not a string."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error = $"{expression.Children[0]} is not a string."; [](start = 20, length = 53)
Why did you drop this? At runtime you can up with a non-string. #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was checking to see if the object returned was not a string. The TryEvaluate() will coerce the result to a string, aka, the number 31 => "31".
In reply to: 367282705 [](ancestors = 367282705)
schemas/baseComponent.schema
Outdated
@@ -34,8 +34,6 @@ | |||
"description": "Extra information for the Bot Framework Designer." | |||
}, | |||
"expression": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should delete this. #Resolved
@@ -12,7 +12,7 @@ | |||
{ | |||
"$kind": "Microsoft.SetProperty", | |||
"property": "$Bread", | |||
"value": "@BreadEntity" | |||
"value": "=@BreadEntity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=@BreadEntity [](start = 22, length = 13)
Once this is checked-in, I'll need to update the generator. #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
} | ||
|
||
public ObjectExpression(string expressionOrString) | ||
: base(expressionOrString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing comments.
{ | ||
} | ||
|
||
public static implicit operator ValueExpression(string valueOrExpression) => new ValueExpression(valueOrExpression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static [](start = 8, length = 13)
Do we not have to document public statics? Missing here and a couple of other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3 |
It basically boils down to how to deal with strings as input:
StringExpression
Interpretation of string is as a string interpolation result unless =.
BoolExpression
Interpretation of string is as a bool expression
IntExpression
Interpretation of string is as a int expression
FloatExpression
Interpretation of string is as a float expression
ExpressionProperty
Interpretation of string is as a T expression
ValueExpression (T == object)
Interpretation of string is as a string interpolation result unless =.
Also Fixes Bugs
#3166 Luis settings not parsed correctly
#3236 Use dialogId as an expression
#3219 HttpRequest body not parsed correctly