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

Tests for double and decimal CellValue ctors #906

Merged
merged 10 commits into from
May 13, 2021
Merged

Tests for double and decimal CellValue ctors #906

merged 10 commits into from
May 13, 2021

Conversation

tomjebo
Copy link
Collaborator

@tomjebo tomjebo commented Mar 5, 2021

  • adding tests for double and decimal ctors for CellValue

Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

Can you update the title of the PR title with details?

public void CellDoubleCultureTest()
{
// Change current culture
CultureInfo culture, oldculture;
Copy link
Member

Choose a reason for hiding this comment

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

can you create a helper for this? Something like this:

internal readonly struct CultureInfoTester:IDisposable
{
  private readonly CultureInfo _old;
  
  public CultureInfoTester(CultureInfo desired)
  {
    _old = Thread.CurrentThread.CurrentCulture;
    Thread.CurrentThread.CurrentCulture = desired;
  }
  
  public void Dispose(){
    Thread.CurrentThread.CurrentCulture = _old;
  }
}

This can then be used to wrap the methods

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So something like this? I just pushed the helper.

// Change current culture
CultureInfo culture, oldculture;
oldculture = Thread.CurrentThread.CurrentCulture;
culture = CultureInfo.CreateSpecificCulture("fr-FR");
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have a few cultures to verify this: ie en-US, fr-FR, ru-RU, zh-CH, tr-TR, ar-EG, etc (get in different ways of handling commas, left-to-right, etc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'll spend some more time on this to see what makes sense.

@@ -74,6 +76,26 @@ public void CellDoubleTest(double num)
Assert.Equal(num, result);
}

[Fact]
public void CellDoubleCultureTest()
Copy link
Member

Choose a reason for hiding this comment

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

what about for int/dates/etc? May not be as big of a deal, but with exponential support added in, it may

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dates definitely could be included. Not sure about int but I'll take some time to check them out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked dates and they don't work the same way as decimal and double numbers. I tested a couple different dates but Excel seems to pay attention to the incoming format, unlike decimal and double.

@tomjebo tomjebo changed the title Issue903 Tests for double and decimal ctors for CellValue per Issue #903 Mar 8, 2021
@tomjebo tomjebo changed the title Tests for double and decimal ctors for CellValue per Issue #903 Tests for double and decimal CellValue ctors Mar 8, 2021
@twsouthwick
Copy link
Member

Any movement here, @tomjebo?

Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

Yeah, that's the right idea. Forgot to put usings around it though

twsouthwick
twsouthwick previously approved these changes Apr 22, 2021
@twsouthwick
Copy link
Member

/azp run

@tomjebo tomjebo merged commit 6de7de8 into dotnet:main May 13, 2021
@tomjebo tomjebo deleted the issue903 branch October 26, 2021 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants