Skip to content

Deprecated StringExtensions.AsFormat extension #3798

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

Merged

Conversation

Sergio0694
Copy link
Member

PR Type

What kind of change does this PR introduce?

  • Deprecation

What is the current behavior?

The Microsoft.Toolkit.StringExtensions.AsFormat extension can be replaced with C# string interpolation and is not needed.

What is the new behavior?

The extension has been marked as deprecated.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@Sergio0694 Sergio0694 added extensions ⚡ maintenance ⚙️ .NET Components which are .NET based (non UWP specific) labels Mar 1, 2021
@Sergio0694 Sergio0694 added this to the 7.0 milestone Mar 1, 2021
@ghost ghost added the in progress 🚧 label Mar 1, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio March 1, 2021 22:29
@@ -145,6 +146,7 @@ public static string FixHtml(this string html)
/// <param name="format">The format of the string being linked.</param>
/// <param name="args">The object which will receive the linked String.</param>
/// <returns>Truncated string.</returns>
[Obsolete("This method will be removed in a future version of the Toolkit. Use the native C# string interpolation syntax instead, see: https://docs.microsoft.com/dotnet/csharp/language-reference/tokens/interpolated")]
public static string AsFormat(this string format, params object[] args) => string.Format(format, args);
Copy link
Member

Choose a reason for hiding this comment

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

I think I had added this for x:Bind, does String Interopolation work there?

Copy link
Member Author

@Sergio0694 Sergio0694 Mar 2, 2021

Choose a reason for hiding this comment

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

Oh, I see. So, it should work, and it does, but it's currently bugged (see microsoft/microsoft-ui-xaml#2654).

For instance, this works fine:

<TextBlock Text="{x:Bind x:String.Format('Name: {0}', ViewModel.Name)}" />

But using more arguments results in a build-time error from the XAML compiler because it's doing an incorrect overload resolution (basically it's just picking whatever the first overload is alphabetically), so it ends up picking the one taking an IFormatProvider instead of a string, and then failing to build 😟

Let me know how you'd like to proceed, I'm fine with closing this if you think the extension should remain there for this reason. I wish this was just fixed on the XAML compiler end, as the current overload resolution is really not great.

Copy link
Member

Choose a reason for hiding this comment

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

So, will this helper work around this bug or face the same issue?

If it helps get around the issue, then maybe let's leave it for now until we know that we can get a fix in WinUI 3?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well this is a bit annoying, I just checked with this extension and it seems the XAML compiler still chokes up anyway because it doesn't account for a params T[] parameter allowing multiple inputs. As in, this fails too:

<TextBlock Text="{x:Bind wct:StringExtensions.AsFormat('{0} {1}', ViewModel.Name, ViewModel.Surname)}"/>

With this error:

Invalid binding path 'wct:StringExtensions.AsFormat('{0} {1}', ViewModel.Name, ViewModel.Surname)' : Missmatched function parameter count.

It just works with a single parameter basically, just with string.Format as usual. It seems to me due to the quirks of the XAML compiler we can then basically deprecate the extension without losing any actual functionality? 🤔

Copy link
Member

@michael-hawker michael-hawker Mar 2, 2021

Choose a reason for hiding this comment

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

@Sergio0694 yeah, let's deprecate, but point to the WinUI issue for reference in addition.

Copy link
Member Author

@Sergio0694 Sergio0694 Mar 3, 2021

Choose a reason for hiding this comment

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

Not sure where to add the link to the WinUI issue, since that's XAML specific and the Microsoft.Toolkit is platform agnostic instead (as in, I thought we wanted to avoid platform specific notes in these packages). Do you want me to just leave a link here in the PR or should I add that to the XML docs of the extension anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just in the code then vs. anything publicly visible? Just for our own knowledge within the toolkit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done in 1fb8c50 🙂

@Sergio0694 Sergio0694 force-pushed the deprecate/string-format branch from f825db3 to bc98bf7 Compare March 2, 2021 21:52
@Sergio0694 Sergio0694 force-pushed the deprecate/string-format branch from bc98bf7 to 76787ed Compare March 3, 2021 15:54
@michael-hawker michael-hawker merged commit 66c9743 into CommunityToolkit:master Mar 3, 2021
@ghost ghost added Completed 🔥 and removed in progress 🚧 labels Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Completed 🔥 extensions ⚡ maintenance ⚙️ .NET Components which are .NET based (non UWP specific)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants