Datetime support for multiple format

[TheBigRoomXXL]

Adding a function datetime2properties to support the differents formats of marshamllow.field.DateTime.
An exemple for custom DateTime format has been added to the doc in Using Plugins / Marshmallow Plugin

The support for timestamp and timestamp_ms mean that DateTime type can be either "string" or "integer". Format can also have multiple values. Because of that 2 test where modified to account for that variability:

• test_field2property_type
• test_field2property_formats

Then 6 tests were added to test the different output of datetime2properties.

[TheBigRoomXXL]

I am sorry but do you need something more before merging? I don't really know why this pull request stay open if there is no problem with it.

[lafrech]

Simply lack of benevolent maintainer time. Please keep this open. I just can't promise when it will be reviewed. To anyone, feel free to chime in and help with the review.

PRs that are obviously wrong are closed, PRs that are trivially good are merged. Other PRs take time to review. Sorry about that. Thanks for contributing.

[Bangertm]

The one thing I'm thinking about with this code is whether it should be more closely integrated with field2type_and_format. The first condition is leaving the results of of field2type_and_format in place, which was a bit confusing when I first looked at it, because you have to scan up a few hundred lines to figure out what's going on.

It might be better to remove DateTime from the field mapping and call this function from field2type_and_format for that special case.

[TheBigRoomXXL]

@lafrech Thank you for your fast feeback, I just needed to know if there was something expected of me.

@Bangertm You are right, finding the result of the first condition might be a little confusing. I see two options to fix that:

1 - just add a comment explaning the expected result:

if field.format == "iso" or field.format is None:
  # Will return { "type": "string", "format": "date-time" } as specified inside DEFAULT_FIELD_MAPPING
  pass

2 - Force the result here (as you proposed if I understood you correctly?):

if field.format == "iso" or field.format is None:
ret = {
  "type": "string",
  "format": "date-time",
}

The funny thing is that if you do that you suddenly break the tests for date format. It looks like isinstance(field, marshmallow.fields.DateTime) return true if field of type marshmallow.fields.Date. I guess it kind of make sense but it is a little weird and I don't know if it's expected behavior.

To avoid any confusion in the future, it would probably be wise to and not isinstance(field, marshmallow.fields.Date) to the first if statement (line 527).

Regarding the original problem I think the option 1 is best as it doesn't break the usual flow and is still quite clear.

[TheBigRoomXXL]

So I made 3 changes to the original merge request:

• I added and not isinstance(field, marshmallow.fields.Date ) to avoid any false positive
• I added a comment describing the output of the iso format so that it doesn't override any existing logic.
• I changed {"type": "integer", "format": "timestamp",} to {"type": "number", "format": "float",} because marshamllow return float, not integer.

[lafrech]

Thanks! Sorry about the delay. Just a few minor questions/comments.

[TheBigRoomXXL]

Thank you for your review. I will be more reactive this time if you need any other change!

[sloria]

Made some minor fixups so this is mergeable. Thanks so much for the contribution @TheBigRoomXXL!