Tests should be aware of the Current Culture Info #34
Conversation
|
Take a look at the diff, there seems to be a whitespace issue. |
|
Hell, yes it seems, i'm on it. |
|
#35 #36 @mellinoe |
|
Looks better now, thanks ! |
|
Thanks for this. Please squash this in to a single commit so that there will be no spaces -> tabs -> spaces noise in the master history if we do merge it. |
| Vector2 v1 = new Vector2(2.0f, 3.0f); | ||
|
|
||
| string v1str = v1.ToString(); | ||
| Assert.Equal("<2, 3>", v1str); | ||
| string expectedv1 = string.Format(CultureInfo.CurrentCulture, "<{0:G}" + separator + "{1:G}>", 2, 3); |
There was a problem hiding this comment.
Could you keep one of these using en-us culture? I think there's some value in at least testing with one explicit culture here. As is, both of these calls will use CultureInfo.CurrentCulture (no-params ToString calls with CultureInfo.CurrentCulture). Same applies to the other tests on the other types.
There was a problem hiding this comment.
Yes sure, i have an argument with myself, "should i keep explicit culture or not...".
|
Hey jbuiss0n, thanks for the PR. It looks good overall, but I made a few small notes before we can merge it in. Thanks for fixing the tabs, by the way! |
|
|
||
| string v2strformatted = v1.ToString("c"); | ||
| Assert.Equal("<$2.50, $2.00, $3.00, $3.30>", v1strformatted); | ||
| string expectedv2formatted = string.Format(CultureInfo.CurrentCulture | ||
| , "<{0:c}" + separator + "{1:c}" + separator + "{2:c}" + separator + "{3:c}>" |
There was a problem hiding this comment.
All these concatenations can be replaced with something like:
string.Format(CultureInfo.CurrentCulture, "<{1:c}{0} {2:c}{0} {3:c}{0} {4:c}>", separator, 2.5, 2, 3, 3.3);
There was a problem hiding this comment.
Agreed, although make sure there is still only a single space between elements if you go this route.
There was a problem hiding this comment.
Yes, the separator variable should not have a space appended to it, that should be done in the format.
|
The latest commits look good to me (I made one comment about formatting in the diff). If you don't mind, we'd like to get this compressed to a single commit before we merge it in. |
…on en-US culture environment
|
Remember; this is git - you can pull down the branch, squash/touch up the commits, merge and push it, while keeping the original author 😉 |
|
@khellang Thanks for the suggestion. We realize that we can technically do that, but we'd prefer to ask the author to rebase because otherwise it:
I also generally dislike rebasing commits authored by someone else because I would prefer the history to reflect what individuals actually intentionally committed on their own. Where do you draw the line? What's OK to modify under someone else's name and what's not, etc. I feel that the incoming history represents part of the contribution and it's not too much to ask contributors to take the time to factor it appropriately. This is exactly what we'd ask of any member of our team. |
jbuiss0n
force-pushed
the
fix-vectors-tests
branch
from
8d2e2e2
to
1e081ed
Compare
|
Everything should be ok now ! |
|
nice job @jbuiss0n! |
|
Thanks guys, it's a pleasure. |
|
Thanks a bunch @jbuiss0n, merged this in just now. |
Clean up SocketAsyncEventArgs.
Xml with cli in progress2.1
Hello,
As i'm French, my CultureInfo.CurrentCulture is set to "fr-FR" while running tests.
But for some tests, especially for the ToString() methods, tests are write to check "en-US" Culture.