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

[Templating] Expose warnings/errors to callers #7437

Merged
merged 12 commits into from
May 11, 2022

Conversation

anna-dingler
Copy link
Contributor

@anna-dingler anna-dingler commented May 5, 2022

Related Issue

Fixes #7433

Description

TLDR

Warnings need to be exposed to the caller instead of logged in the console.

Proposed Solution

C#
Add optional out parameters to the AdaptiveCardTemplate.Expand methods that's an array of warning strings

JS
Add an overload for the Template.expand method that takes an out parameter that's an array of warning strings (or alternatively, maybe an optional out parameter?)

In this PR

I added an optional parameter for an array of warning/error strings to the expand method of the JS library.

For the .NET library, I added an out parameter for the warning/error string array to the Expand methods. I also added an additional wrapper methods, that call the updated Expand methods to ensure this isn't a breaking change.

Sample Card

These cards should produce warning strings if the proper expand method is called.

Test 1 - Not an Expression

{
    "type": "AdaptiveCard",
    "body": [
        {
            "type": "TextBlock",
            "size": "Medium",
            "weight": "Bolder",
            "text": "${title}",
            "$when": "NotAnExpression"
        }
    ],
    "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
    "version": "1.5"
}

Test 2 - Invalid Expression No Data

{
    "type": "AdaptiveCard",
    "body": [
        {
            "type": "TextBlock",
            "size": "Medium",
            "weight": "Bolder",
            "text": "${title}",
            "$when": "${InvalidExpression}"
        }
    ],
    "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
    "version": "1.5"
}

Test 3 - Invalid Expression with Data

Card

{
    "type": "AdaptiveCard",
    "body": [
        {
            "type": "TextBlock",
            "size": "Medium",
            "weight": "Bolder",
            "text": "${title}",
            "$when": "${InvalidExpression}"
        }
    ],
    "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
    "version": "1.5"
}

Data

{
   "test": "dummyData"
}

How Verified

Verified with the unit tests added with this PR.

Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented May 5, 2022

Hi @anna-dingler. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

Copy link
Member

@jwoo-msft jwoo-msft left a comment

Choose a reason for hiding this comment

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

we have a lot of Expand methods, can you somehow consolidate it. at minimum, can you make sure the primary Expand method is at the prominent place, so devs would know which one is the primary one without much of a search?
and if you could, it would be nice if we can reduce the number ArrayList instantiation. So if we are going to change log data structure, we could do it at one place.

@anna-dingler
Copy link
Contributor Author

we have a lot of Expand methods, can you somehow consolidate it. at minimum, can you make sure the primary Expand method is at the prominent place, so devs would know which one is the primary one without much of a search? and if you could, it would be nice if we can reduce the number ArrayList instantiation. So if we are going to change log data structure, we could do it at one place.

I grouped the wrapper methods together and left additional comments to the Expand methods to make it clearer. I also used _ to indicate a discarded out parameter to cut down on ArrayList instantiation.

Should I be committing the autogenerated .md files fot the .NET library?

@anna-dingler anna-dingler marked this pull request as ready for review May 6, 2022 19:54
@anna-dingler anna-dingler merged commit 1b3b494 into microsoft:main May 11, 2022
@anna-dingler anna-dingler deleted the templatingExposeWarnings branch May 11, 2022 17:01
anna-dingler added a commit that referenced this pull request May 11, 2022
* In progress: log warnings

* Error log for .NET

* Add error logging for NodeJS

* Resolve comments on PR

* Consolidate ArrayList declarations and fix testing nuget package

* Rearrange Expand methods and add comments

* Refactor to add the errors to template instance

* Update warning message

* Get warnings from last template expansion

* Add markdown file changes
anna-dingler added a commit that referenced this pull request May 12, 2022
* In progress: log warnings

* Error log for .NET

* Add error logging for NodeJS

* Resolve comments on PR

* Consolidate ArrayList declarations and fix testing nuget package

* Rearrange Expand methods and add comments

* Refactor to add the errors to template instance

* Update warning message

* Get warnings from last template expansion

* Add markdown file changes
paulcam206 added a commit that referenced this pull request May 13, 2022
* [.NET Templating] Update Boolean expression evaluation (#7223)

* Update expansion of Boolean expressions

* Add unit test

* Add test for property that accepts boolean

* [.NET Templating] Resolve commas added in wrong place (#7225)

* Update expansion of Boolean expressions

* Add unit test

* Add test for property that accepts boolean

* Resolve extra commas in temaplting output

* [Templating] Realign platforms on invalid $when behavior (#7432)

* Align templating platforms for invalid boolean behavior

* Formatting

* Add comment for exposing warnings to caller

* [Templating] Expose warnings/errors to callers (#7437)

* In progress: log warnings

* Error log for .NET

* Add error logging for NodeJS

* Resolve comments on PR

* Consolidate ArrayList declarations and fix testing nuget package

* Rearrange Expand methods and add comments

* Refactor to add the errors to template instance

* Update warning message

* Get warnings from last template expansion

* Add markdown file changes

Co-authored-by: Paul Campbell <paulcam@microsoft.com>
michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
* In progress: log warnings

* Error log for .NET

* Add error logging for NodeJS

* Resolve comments on PR

* Consolidate ArrayList declarations and fix testing nuget package

* Rearrange Expand methods and add comments

* Refactor to add the errors to template instance

* Update warning message

* Get warnings from last template expansion

* Add markdown file changes
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.

[Templating] Expose errors/warnings to callers
3 participants