-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Deprecated StringExtensions.AsFormat extension #3798
Conversation
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 🙌 |
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done in 1fb8c50 🙂
f825db3
to
bc98bf7
Compare
bc98bf7
to
76787ed
Compare
PR Type
What kind of change does this PR introduce?
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:
Pull Request has been submitted to the documentation repository instructions. Link:Sample in sample app has been added / updated (for bug fixes / features)Icon has been created (if new sample) following the Thumbnail Style Guide and templates