Maui icon indicating copy to clipboard operation
Maui copied to clipboard

Migrate to PEG parser. Introduce boolean operators and constants.

Open stephenquan opened this issue 1 year ago • 7 comments

Description of Change

  • MultiMathExpressionConverterPage.xaml
    • Add VerticalStackLayout and move existing sample inside
    • Added a Label with a DataTrigger on "x0 + x1 + x2 >= 60" to demonstrate the comparison operator
  • MathOperator.shared.cs
    • Replaced double with object
    • MathOperatorPrecedence obsolete (part of the original parsing engine but no longer relevant in new engine)
  • MathExpressionConverterTests.cs
    • New tests for comparison, equality, and boolean operators
    • Certain operations allow nulls now
  • MathExpression.shared.cs
    • Refactor/rewrite to use Parsing expression grammar
    • The grammar parity with original operators +, -, *, / and ^ as well as math functions
    • Added to grammar and, or, ?, :, >=, >, <=, <, ==, != as well as true and false constants
    • Relax null strictness (e.g. equality and ternary operators can have nulls)
  • MathExpressionConverter.shared.cs
    • Replaced double with object
  • MultiMathExpressionConverter.cs
    • Relax null strictness (e.g. equality and ternary operators can have nulls)

General comments:

  • Original implementation sanitizes all inputs by calling double.Parse(value.ToString) rejecting nulls
  • New implementation calls Convert.ToDouble() when needed and, in certain situations, allows nulls
  • When a boolean value is needed, boolean coercion uses the following rules:
    • If value is null, return false
    • If value is a boolean, return value
    • If value is a double, return true if value != 0 && value != NaN, false otherwise
    • If value is a string, return true, if value is not null or empty, false otherwise

Linked Issues

https://github.com/CommunityToolkit/Maui/issues/2181

PR Checklist

  • [x] Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • [x] Has tests (if omitted, state reason in description)
  • [x] Has samples (if omitted, state reason in description)
  • [x] Rebased on top of main at time of PR
  • [x] Changes adhere to coding standard
  • [x] Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pull/465

Additional information

stephenquan avatar Sep 05 '24 22:09 stephenquan

@dotnet-policy-service agree company="Esri"

stephenquan avatar Sep 05 '24 23:09 stephenquan

Hi @brminnick since your review, I have:

  • attempted to update the fork with the base branch
  • corrected the unit test issues you raised
  • added more unit test for better coverage of the boolean operators
  • updated the handling of nulls

The fork is ready for another round of review

stephenquan avatar Sep 16 '24 23:09 stephenquan

@stephenquan here's the build log:

D:\a\1\s\src\CommunityToolkit.Maui\Converters\MathExpressionConverter\MathExpressionConverter.shared.cs(25,10): error CS8603: Possible null reference return. [D:\a\1\s\src\CommunityToolkit.Maui\CommunityToolkit.Maui.csproj::TargetFramework=net8.0-ios]
D:\a\1\s\src\CommunityToolkit.Maui\Converters\MathExpressionConverter\MathExpressionConverter.shared.cs(25,10): error CS8603: Possible null reference return. [D:\a\1\s\src\CommunityToolkit.Maui\CommunityToolkit.Maui.csproj::TargetFramework=net8.0-windows10.0.19041.0]
D:\a\1\s\src\CommunityToolkit.Maui\Converters\MathExpressionConverter\MathExpressionConverter.shared.cs(25,10): error CS8603: Possible null reference return. [D:\a\1\s\src\CommunityToolkit.Maui\CommunityToolkit.Maui.csproj::TargetFramework=net8.0-tizen]
D:\a\1\s\src\CommunityToolkit.Maui\Converters\MathExpressionConverter\MathExpressionConverter.shared.cs(25,10): error CS8603: Possible null reference return. [D:\a\1\s\src\CommunityToolkit.Maui\CommunityToolkit.Maui.csproj::TargetFramework=net8.0-maccatalyst]
D:\a\1\s\src\CommunityToolkit.Maui\Converters\MathExpressionConverter\MathExpressionConverter.shared.cs(25,10): error CS8603: Possible null reference return. [D:\a\1\s\src\CommunityToolkit.Maui\CommunityToolkit.Maui.csproj::TargetFramework=net8.0-android]
  

pictos avatar Sep 17 '24 01:09 pictos

Thanks @pictos, I have reviewed the CS8063 possible null reference return issues and have resolved it by making nullability updates to both the MathExpression.shared.cs and MathExpressionConverter.shared.cs. The nullability updates also required additional Asserts added to the MathExpressionConverterTests.cs. With these changes, the build checks are now passing.

stephenquan avatar Sep 18 '24 04:09 stephenquan

@pictos @bijington I have pushed updates to the fork address both the C# property name pattern and logging invalid math expressions with TraceWarning. Because we're no longer raising invalid math expressions I had to change the relevant unit test from an exception test to a null check. The fork is ready for review again.

stephenquan avatar Sep 25 '24 02:09 stephenquan

@pictos @brminnick as requested, I have removed the Null Forgiving Operator from the MultiMathExpressionConverter. I have corrected the null checks in the MultiMathExpressionConverter and that resulted in a necessary minor update to the unit tests.

This fork is ready for review again.

stephenquan avatar Oct 06 '24 23:10 stephenquan

Hi all, we are using snapshot copies of these classes in our projects. I am eager to help out and remove any blockers on this PR.

stephenquan avatar Oct 20 '24 21:10 stephenquan

Hi guys, I think this PR is good to merge.

stephenquan avatar Nov 26 '24 20:11 stephenquan

Hi guys, I think this PR is good to merge.

Thanks. We are trying to get to the point of merging #2215 before any other PRs due to that size of it and that it provides .NET 9.0 support. Then we would ideally merge PRs like this. @brminnick is that correct order for our plan?

bijington avatar Nov 26 '24 20:11 bijington

Edit: Yup - Shaun is correct. I see we commented at the same time 😊 🙌

We are blocked from merging any PRs until we've merged our PR adding support for .NET 9.

Our .NET 9 PR is complete, however, it is blocked by this bug in .NET MAUI: https://github.com/dotnet/maui/issues/25871

It may encourage the MAUI engineering team to increase its priority if you navigate to the .NET MAUI bug blocking our .NET 9 PR and leave a comment asking the team to prioritize it.

TheCodeTraveler avatar Nov 26 '24 20:11 TheCodeTraveler

I have some new developers working with Boolean operators in XAML, but they struggled with the syntax.

To simplify I things added new commits which adds XAML-friendly alternatives. Additionally, I added more coverage to the logical AND/OR unit tests.

Unfortunately, in doing so, 3 of the 6 automated test are failing and I am not sure why. Also, the PR will now require a re-review.

Operator XAML Alternate
&& &amp;&amp; and
|| || or
>= &gt;= ge
> &gt; gt
<= &lt;= le
< &lt; lt

Here are some examples of the alternate syntax:

Original Alternate
x &gt;= 0 ? x1 : x2 x ge 0 ? x1 : x2
x &amp;&amp; !x1 x and !x1
x &gt;= 0 &amp;&amp; x &lt;= 10 x ge 0 and x le 10

stephenquan avatar Dec 05 '24 10:12 stephenquan

The azure pipelines https://dev.azure.com/dotnet/CommunityToolkit/_build/results?buildId=112270&view=logs&j=67da6a11-18ef-5f70-9b43-fae7e6c83e18&t=54ba8226-26d8-53c8-39e8-392f99f22929&l=538 is showing 11 new errors that I do not know how to resolve. These 11 new errors do not relate the commits done, so, I'm hoping they will automatically resolve when the fork catches up with the main branch:

D:\a\1\s\samples\CommunityToolkit.Maui.Sample\MauiProgram.cs(38,20): error CS0234: The type or namespace name 'Composition' does not exist in the namespace 'Microsoft.UI' (are you missing an assembly reference?) [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-windows10.0.19041.0] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\MauiProgram.cs(39,20): error CS0234: The type or namespace name 'Windowing' does not exist in the namespace 'Microsoft.UI' (are you missing an assembly reference?) [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-windows10.0.19041.0] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\MauiProgram.cs(40,25): error CS0234: The type or namespace name 'Media' does not exist in the namespace 'Microsoft.UI.Xaml' (are you missing an assembly reference?) [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-windows10.0.19041.0] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\Platforms\Windows\App.xaml.cs(6,28): error CS0246: The type or namespace name 'MauiWinUIApplication' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-windows10.0.19041.0] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\Platforms\MacCatalyst\AppDelegate.cs(6,28): error CS0246: The type or namespace name 'MauiUIApplicationDelegate' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-maccatalyst] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\Platforms\iOS\AppDelegate.cs(6,28): error CS0246: The type or namespace name 'MauiUIApplicationDelegate' could not be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-ios] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\obj\Release\net8.0-android\lp\148\jl\res\layout\tabbar.xml(2): error APT2260: attribute tabGravity (aka com.microsoft.CommunityToolkit.Maui.Sample:tabGravity) not found. [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\obj\Release\net8.0-android\lp\148\jl\res\layout\tabbar.xml(2): error APT2260: [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\obj\Release\net8.0-android\lp\148\jl\res\layout\tabbar.xml(2): error APT2260: attribute tabIndicatorColor (aka com.microsoft.CommunityToolkit.Maui.Sample:tabIndicatorColor) not found. [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\obj\Release\net8.0-android\lp\148\jl\res\layout\tabbar.xml(2): error APT2260: [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\obj\Release\net8.0-android\lp\148\jl\res\layout\tabbar.xml(2): error APT2260: attribute tabMode (aka com.microsoft.CommunityToolkit.Maui.Sample:tabMode) not found. [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] D:\a\1\s\samples\CommunityToolkit.Maui.Sample\obj\Release\net8.0-android\lp\148\jl\res\layout\tabbar.xml(2): error APT2260: [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] C:\hostedtoolcache\windows\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.145\tools\Xamarin.Android.Aapt2.targets(156,3): error APT2061: failed linking file resources. [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android] C:\hostedtoolcache\windows\dotnet\packs\Microsoft.Android.Sdk.Windows\34.0.145\tools\Xamarin.Android.Aapt2.targets(156,3): error APT2061: [D:\a\1\s\samples\CommunityToolkit.Maui.Sample\CommunityToolkit.Maui.Sample.csproj::TargetFramework=net8.0-android]

stephenquan avatar Dec 06 '24 08:12 stephenquan

Hi @stephenquan! Could you update this PR to resolve the merge conflicts?

We've finally merged the massive .NET 9 PR today and it introduced a few merge conflicts across our open PRs.

TheCodeTraveler avatar Dec 18 '24 01:12 TheCodeTraveler