-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fixed StringFormatConverter ignoring Language parameter, cleanup #4764
Fixed StringFormatConverter ignoring Language parameter, cleanup #4764
Conversation
Thanks Arlodotexe 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 🙌 |
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 if we can add a better test case here based on how this works or could be compromised if something in the platform/world changes (though think that would be unlikely).
Small optimization though for the converter itself though which I think could be a good change.
Assert.AreEqual(Date.ToString(expectedFormat, CultureInfo.InvariantCulture), result); | ||
} | ||
|
||
[TestCategory("Converters")] |
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.
Is there a test we could add like a general date where we have the same format string but then change the language between en/gb and see the result of the date with month/day flipped? Then we know for sure it worked, right?
A lot of the current test cases just seem to be testing string.Format
to ToString
in a way we know should work, right? (which is important, but not really replicating how we expect it to be used in an app, right?)
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.
@Arlodotexe you could have left them, was just suggesting an additional test, looks like something was missed in the clean-up:
D:\a\1\s\UnitTests\UnitTests.UWP\Converters\Test_StringFormatConverter.cs(19,41): error CS0414: The field 'Test_StringFormatConverter.Amount' is assigned but its value is never used [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Fixes #3243
This PR pulls in the changes from #4546, and cleans up a significant amount of plumbing that isn't needed. All unit tests are passing, and new tests have been added.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
In
IValueConverter
, thelanguage
parameter is being ignored.That means passing a language via
ConverterLanguage
wouldn't format the string with respect to the provided language.What is the new behavior?
The language property is being passed to the string formatter. Now, the following will format as expected:
Example usage:
PR Checklist
Please check if your PR fulfills the following requirements: