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

Added fix to support spaces between the method name and its () to prevent a false positive MCTME001 error #1477

Merged
merged 42 commits into from
Sep 18, 2024

Conversation

SotoiGhost
Copy link
Contributor

@SotoiGhost SotoiGhost commented Oct 26, 2023

Description of Change

Added fix to support spaces between the method name and its () to prevent a false positive MCTME001 error

Linked Issues

PR Checklist

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

Additional information

Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on the PR @SotoiGhost!

Could you please refactor this to include two things:

  1. Use the [GeneratedRegex] attribute
  2. Add unit tests to CommunityToolkit.Maui.Analyzers.UnitTests that demonstrate both use-cases
    • Add a unit test for UseMauiCommunityToolkit()
    • Add a second unit test for UseMauiCommunityToolkit ()

I see now that we don't have a CommunityToolkit.Maui.MediaElement.Analyzers.UnitTests project. We totally should, but missed it when creating MediaElement.

It'd be great if you could create the CommunityToolkit.Maui.MediaElement.Analyzers.UnitTests project too and include the same unit tests that you'll add to CommunityToolkit.Maui.Analyzers.UnitTests! But no worries if you don't; that's not your fault it's ours, and I won't block this PR if it doesn't include it.

More Info on GeneratedRegex

This attribute generates the RegEx code at compile time to improve performance at runtime using RegEx.

Here's an example of how we are using GeneratedRegex in the toolkit:

[GeneratedRegex(@"^[^@\s]+@[^@\s]+\.[^@\s]+$", RegexOptions.IgnoreCase, 250)]

And here is Microsoft's guidance from the docs:
https://learn.microsoft.com/dotnet/standard/base-types/regular-expression-source-generators#source-generation

The general guidance is if you can use the source generator, use it. If you're using Regex today in C# with arguments known at compile time, and especially if you're already using RegexOptions.Compiled (because the regex has been identified as a hot spot that would benefit from faster throughput), you should prefer to use the source generator. The source generator will give your regex the following benefits:

  • All the throughput benefits of RegexOptions.Compiled.
  • The startup benefits of not having to do all the regex parsing, analysis, and compilation at run time.
  • The option of using ahead-of-time compilation with the code generated for the regex.
  • Better debuggability and understanding of the regex.
  • The possibility to reduce the size of your trimmed app by trimming out large swaths of code associated with RegexCompiler (and potentially even reflection emit itself).

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.

Instead of using regex, can you use the Roslyn APIs to remove any trivia, like whitespaces?

@SotoiGhost
Copy link
Contributor Author

@brminnick I was trying to add the [GeneratedRegex] but I realized that the Analyzer project is a netstandard2.0 and the attribute is only available on .net7 and .net8.

image

@pictos I don't have experience with the Roslyn APIs but if you can point me into the right direction, I can make the changes, meanwhile, I'm trying to add unit testing. It's always good to learn new things! 😃

@pictos
Copy link
Member

pictos commented Nov 6, 2023

@SotoiGhost I don't have EOT to look into that... To not block this PR, we can merge it and in the future, I'll find time to investigate removing the regex.

@bijington
Copy link
Contributor

We won't merge this until we have proven the Regex doesn't impact build performance. I have it on my list to look at 👍

@ghost ghost added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Dec 7, 2023
@vhugogarcia
Copy link
Contributor

We won't merge this until we have proven the Regex doesn't impact build performance. I have it on my list to look at 👍

.NET 8 includes a number of Regex performance improvements, including support for compiling regular expressions more efficiently and support for executing regular expressions more efficiently.

By using this pattern: @"\.UseMauiCommunityToolkit\s*\(\)" . This pattern breaks down as follows:

.: Escapes the dot since it’s a special character in Regex, ensuring it matches a literal dot.
UseMauiCommunityToolkit: Matches the literal text “UseMauiCommunityToolkit”.
\s*: Matches any whitespace character (like space, tab, etc.) zero or more times. This accounts for both no space and the space in “UseMauiCommunityToolkit ()”.
(): Matches the literal parentheses.

This Regex is performant because it avoids unnecessary backtracking and matches both text options efficiently. Here’s how you might use it in code:

using System.Text.RegularExpressions;

string pattern = @"\.UseMauiCommunityToolkit\s*\(\)";
Regex regex = new Regex(pattern, RegexOptions.Compiled);

bool isValidOption1 = regex.IsMatch(".UseMauiCommunityToolkit()");
bool isValidOption2 = regex.IsMatch(".UseMauiCommunityToolkit ()");

This code snippet creates a compiled Regex object and checks if the two text options match the pattern. Both isValidOption1 and isValidOption2 should evaluate to true if the text options are as provided. The RegexOptions.Compiled option is set to this pattern since it will be used frequently, to further improve performance.

What do you all think? cc: @SotoiGhost

I just wanted to share my grain of salt on this PR. 😃

@pictos pictos requested a review from brminnick March 5, 2024 22:41
@brminnick brminnick removed blocked stale The author has not responded in over 30 days labels Sep 17, 2024
@brminnick brminnick removed the help wanted This proposal has been approved and is ready to be implemented label Sep 18, 2024
Copy link
Collaborator

@brminnick brminnick 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 we're good to go!

I've updated the Analyzers to ignore whitespace while improving performance, and I've added comprehensive Unit Test suites to verify the new changes 🚀

@brminnick brminnick enabled auto-merge (squash) September 18, 2024 19:05
@brminnick brminnick added the needs discussion Discuss it on the next Monthly standup label Sep 18, 2024
@brminnick brminnick merged commit cf6360d into CommunityToolkit:main Sep 18, 2024
7 checks passed
@MartyIX MartyIX mentioned this pull request Sep 18, 2024
ne0rrmatrix added a commit to ne0rrmatrix/MauiOld that referenced this pull request Sep 18, 2024
…()` to prevent a false positive MCTME001 error (CommunityToolkit#1477)"

This reverts commit cf6360d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Discuss it on the next Monthly standup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Having a space between the UseMauiCommunityToolkitMediaElement and the ( throws the MCTME001 error
6 participants