Repository navigation
Parse Context Update #3548
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
Parse Context Update #3548
Changes from 6 commits
0bac934
474884a
52e882f
9eb0598
97e3834
ab00f52
6b60262
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,11 @@ | |
| using Newtonsoft.Json.Linq; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Runtime.CompilerServices; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this needed? #Closed
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| namespace AdaptiveCards | ||
| { | ||
| public class AdaptiveFallbackConverter : JsonConverter, ILogWarnings | ||
| public class AdaptiveFallbackConverter : AdaptiveTypedBaseElementConverter, ILogWarnings | ||
| { | ||
| public List<AdaptiveWarning> Warnings { get; set; } = new List<AdaptiveWarning>(); | ||
|
|
||
|
|
@@ -103,7 +104,7 @@ public override bool CanConvert(Type objectType) | |
| return result; | ||
| } | ||
|
|
||
| public static AdaptiveFallbackElement ParseFallback(JToken fallbackJSON, JsonSerializer serializer, string objectId, AdaptiveInternalID internalId) | ||
| public AdaptiveFallbackElement ParseFallback(JToken fallbackJSON, JsonSerializer serializer, string objectId, AdaptiveInternalID internalId) | ||
| { | ||
| // Handle fallback as a string ("drop") | ||
| if (fallbackJSON.Type == JTokenType.String) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| using Newtonsoft.Json; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Should be able to clean up the using's here to remove ones not being used in the file #Resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| namespace AdaptiveCards | ||
| { | ||
| public abstract class AdaptiveTypedBaseElementConverter : JsonConverter | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Late but adding comments anyways; Good to have some class comments explainng why this wasnt needed for eg. Also do you need System.Threading.Tasks here? #Resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| { | ||
| protected ParseContext parseContext; | ||
| public ParseContext ParseContext | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious: Wouldnt automatic properties work here?Also why is the local member (parseContext) declared as protected? Additionally why is ParseContext - the property a public property vs protected? #Resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. o.k. will change to automatic property. it was intended that inherited object would be able to freely access this property, but automatic property makes more sense. In reply to: 341280836 [](ancestors = 341280836) |
||
| { | ||
| get => parseContext; | ||
| set | ||
| { | ||
| parseContext = value; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| using System.Collections.Generic; | ||
| using System.Collections; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using Newtonsoft.Json; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace AdaptiveCards.Test | ||
| { | ||
|
|
@@ -1244,6 +1244,152 @@ public void TestParsingRichTextBlockWithInvalidInlineType() | |
|
|
||
|
|
||
| StringAssert.Contains(ex.Message, "Property 'type' must be 'TextRun'"); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: newline here to break between functions #Resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| void RenderCardTask(string payload) | ||
| { | ||
| AdaptiveCardParseResult parseResult = AdaptiveCard.FromJson(payload); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void TestConcurrentParsing() | ||
| { | ||
| // card with invalid inline type | ||
| var adaptiveCard = | ||
| @"{ | ||
| ""$schema"": ""http://adaptivecards.io/schemas/adaptive-card.json"", | ||
| ""type"": ""AdaptiveCard"", | ||
| ""version"": ""1.0"", | ||
| ""body"": [ | ||
| { | ||
| ""type"": ""TextBlock"", | ||
| ""text"": ""Publish Adaptive Card schema"", | ||
| ""weight"": ""bolder"", | ||
| ""size"": ""medium"" | ||
| }, | ||
| { | ||
| ""type"": ""ColumnSet"", | ||
| ""columns"": [ | ||
| { | ||
| ""type"": ""Column"", | ||
| ""width"": ""auto"", | ||
| ""items"": [ | ||
| { | ||
| ""type"": ""Image"", | ||
| ""url"": ""https://pbs.twimg.com/profile_images/3647943215/d7f12830b3c17a5a9e4afcc370e3a37e_400x400.jpeg"", | ||
| ""size"": ""small"", | ||
| ""style"": ""person"" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| ""type"": ""Column"", | ||
| ""width"": ""stretch"", | ||
| ""items"": [ | ||
| { | ||
| ""type"": ""TextBlock"", | ||
| ""text"": ""Matt Hidinger"", | ||
| ""weight"": ""bolder"", | ||
| ""wrap"": true | ||
| }, | ||
| { | ||
| ""type"": ""TextBlock"", | ||
| ""spacing"": ""none"", | ||
| ""text"": ""Created {{DATE(2017-02-14T06:08:39Z, SHORT)}}"", | ||
| ""isSubtle"": true, | ||
| ""wrap"": true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| ""type"": ""TextBlock"", | ||
| ""text"": ""Now that we have defined the main rules and features of the format, we need to produce a schema and publish it to GitHub. The schema will be the starting point of our reference documentation."", | ||
| ""wrap"": true | ||
| }, | ||
| { | ||
| ""type"": ""FactSet"", | ||
| ""facts"": [ | ||
| { | ||
| ""title"": ""Board:"", | ||
| ""value"": ""Adaptive Card"" | ||
| }, | ||
| { | ||
| ""title"": ""List:"", | ||
| ""value"": ""Backlog"" | ||
| }, | ||
| { | ||
| ""title"": ""Assigned to:"", | ||
| ""value"": ""Matt Hidinger"" | ||
| }, | ||
| { | ||
| ""title"": ""Due date:"", | ||
| ""value"": ""Not set"" | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| ""actions"": [ | ||
| { | ||
| ""type"": ""Action.ShowCard"", | ||
| ""title"": ""Set due date"", | ||
| ""card"": { | ||
| ""type"": ""AdaptiveCard"", | ||
| ""body"": [ | ||
| { | ||
| ""type"": ""Input.Date"", | ||
| ""id"": ""dueDate"" | ||
| } | ||
| ], | ||
| ""actions"": [ | ||
| { | ||
| ""type"": ""Action.Submit"", | ||
| ""title"": ""OK"" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| ""type"": ""Action.ShowCard"", | ||
| ""title"": ""Comment"", | ||
| ""card"": { | ||
| ""type"": ""AdaptiveCard"", | ||
| ""body"": [ | ||
| { | ||
| ""type"": ""Input.Text"", | ||
| ""id"": ""comment"", | ||
| ""isMultiline"": true, | ||
| ""placeholder"": ""Enter your comment"" | ||
| } | ||
| ], | ||
| ""actions"": [ | ||
| { | ||
| ""type"": ""Action.Submit"", | ||
| ""title"": ""OK"" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| }"; | ||
| List<Task> tasks = new List<Task>(); | ||
| for (var i = 0; i < 10; i++) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
was this sufficient to trigger errors before your fix? #Closed
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| { | ||
| var payload = adaptiveCard; | ||
| var task = Task.Run(() => { RenderCardTask(payload); }); | ||
| tasks.Add(task); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| Task.WaitAll(tasks.ToArray()); | ||
| } | ||
| catch | ||
| { | ||
| // it's perfectly fine card. if there is exception, it is due to concurreny | ||
| // as it's the only variable. | ||
| Assert.Fail(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Include a message here in case the assertion fails to aid debugging a bit by adding some failure context #Resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch; it wasn't carried over from the stopgap since they were worked at in parrarrell. In reply to: 341284485 [](ancestors = 341284485) |
||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
nit: typo :) #Closed
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.
thanks!
In reply to: 340735553 [](ancestors = 340735553)