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

Limit milliseconds to three digits #741

Merged
merged 4 commits into from
Jul 13, 2020

Conversation

stoiveyp
Copy link
Contributor

Fixes #724 and adds test to ensure that's all that changes
No standard format limits to three digits for milliseconds, but the format is the same as "o" used before in all other regards

@twsouthwick
Copy link
Member

@tomjebo can you review this as it's more up your alley. Is the form consistent with the spec? Let's add the section from the spec for reference

@twsouthwick
Copy link
Member

@stoiveyp can you update the changelog to include this change? You may need to merge the latest changes from master if you don't have a 2.11.1 spot for the changes.

@twsouthwick
Copy link
Member

Also, would it make sense to provide a parse function? We want to get a revision version out which we don't expose new APIs, but we could expose it on the next minor version

@twsouthwick
Copy link
Member

Could we use XmlConvert.ToString instead? Wouldn't that be the same, but not require a custom format string

@stoiveyp
Copy link
Contributor Author

@twsouthwick sorry for the delay - day job has worn me out this week!
Did look at XmlConvert, but it also goes down to too many significant digits in the seconds fraction, so we end up having to customise there too.
I'll make the other changes now though 👍

@stoiveyp
Copy link
Contributor Author

@twsouthwick moved the string conversion to static methods in CellValue and moved the formats to constants
Changelog is also updated

Can't find anything in the OpenXml spec specifically about date time cell values.
So I looked in the W3 Xml Schema documents
https://www.w3.org/TR/xmlschema-2/

All ·minimally conforming· processors ·must· support year values with a minimum of 4 digits (i.e., YYYY) and a minimum fractional second precision of milliseconds or three decimal digits (i.e. s.sss). However, ·minimally conforming· processors ·may· set an application-defined limit on the maximum number of digits they are prepared to support in these two cases, in which case that application-defined maximum number ·must· be clearly documented.

So without comment within the OpenXml spec or Excel documentation - a processor based on the Xml Schema spec would only have to support the three digits we're now outputting.

@twsouthwick
Copy link
Member

LGTM! I've opened #752 to add a parsing method so people don't need to know the value to get a strongly typed value. That will have to go into a minor version (which may end up being the next version... there's a few APIs we'll want to expose soon).

@twsouthwick twsouthwick merged commit de29c46 into dotnet:master Jul 13, 2020
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.

CellValue ctor for DateTime and DateTimeOffset should use proper xml standard format
2 participants