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

ISO8601 compatibility for DateTime ToString #158

Merged
merged 14 commits into from
Oct 11, 2021

Conversation

networkfusion
Copy link
Member

@networkfusion networkfusion commented Oct 8, 2021

Description

Adds DateTime.ToString("o"); so that ISO8601 compatible datetimes can be generated.

Workarounds from the full framework are:

  • uses the char Z instead of the datetime Culture specifier K as (I dont think) we support it.
  • only supports millisecond precision (as we dont support further decimal places, and would be a waste to add extra zeros).

Motivation and Context

Option was missing.

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (work on Unit Tests, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@nfbot
Copy link
Member

nfbot commented Oct 8, 2021

Hi @networkfusion,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

It would be great if you could update the DateTime unit tests to cover this change.

@nanoframework nanoframework deleted a comment from nfbot Oct 8, 2021
@networkfusion
Copy link
Member Author

It would be great if you could update the DateTime unit tests to cover this change.

It is already there, but broken...

//public void DateTime_ToStringTest7()

@josesimoes
Copy link
Member

Can you give it a try and fix whatever is not working there? Only for the string formats that we support, of course. 😁

@networkfusion
Copy link
Member Author

networkfusion commented Oct 8, 2021

Assuming the test I have uncommented and updated is reasonable... All tests now pass!
Of note, it seems we cannot handle sub second precision of over 3dp, and we should really just return 0 for extra digits (for fff + ffff rather than an argument exception in another PR?!). Although... Unless it is meaningful, I dont think we should bother returning extra "0"'s in the .ToString("o"), as that just adds data to things like MQTT messages, which are generally charged for based on size, and almost all decoders should handle that (unless it is done to ensure float precision)!

Allows manual verification.
@networkfusion
Copy link
Member Author

For reference, this is how it is now decoded:
image

@josesimoes
Copy link
Member

The standard doesn't specify any size for the fractional part. It mentions that it should be agreed between parties.
For reference: https://en.wikipedia.org/wiki/ISO_8601

Seems that the 3 places comes from from MS SQL formats (and the fact .NETMF was a lightweight platform).
We can surely change that and go with the full fffffff places following the .NET format (and pretty much everyone else).

@josesimoes
Copy link
Member

Regarding the Unit Test: well... that's the bare minimum there... the call could return "carrots" and the test would still be green . 🤣

It should be improved to do a proper comparison and validate the expected outcome.
Do you want me to look into that, or you'll it?

@networkfusion
Copy link
Member Author

Regarding the Unit Test: well... that's the bare minimum there... the call could return "carrots" and the test would still be green . 🤣

It should be improved to do a proper comparison and validate the expected outcome. Do you want me to look into that, or you'll it?

If you could, that would be awesome.

RE the fffffff, I was more going on MS documentation for o than the ISO standard itself:
image

- MaxSecondsFractionDigits is now in sync with .NET.
- Rework code that handles 'f' format token.
- Adjust GetRealFormat for O and o to follow .NET.
- Change visibility of DateTime.TicksPerMillisecond to allow using it from DateTimeFormat.
- Add UnitTest to properly validate output from  'O' and 'o' specifiers.
- Rework code for GetRandomDateTime in DateTime Units Tests to generate full ticks value.
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Should be OK now, fully compliant with .NET and properly tested.

@josesimoes josesimoes merged commit ebcb832 into develop Oct 11, 2021
@josesimoes josesimoes deleted the develop-datetime-iso8601 branch October 11, 2021 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateTime.ToString does not support "o" for ISO8601
3 participants