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

Migrate to PEG parser. Introduce boolean operators and constants. #2182

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

stephenquan
Copy link

@stephenquan stephenquan commented Sep 5, 2024

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

#2181

PR Checklist

Additional information

@stephenquan
Copy link
Author

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

@stephenquan stephenquan marked this pull request as ready for review September 6, 2024 01:49
@stephenquan
Copy link
Author

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

@pictos
Copy link
Member

pictos commented Sep 17, 2024

@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]
  

@stephenquan
Copy link
Author

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.

Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

I think that we're getting really close to merge this!

@stephenquan
Copy link
Author

stephenquan commented Sep 25, 2024

@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.

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.

4 participants