Skip to content

Add serialization for 'datetime'#129

Open
malthe wants to merge 2 commits into
jborean93:masterfrom
malthe:add-datetime-serialization
Open

Add serialization for 'datetime'#129
malthe wants to merge 2 commits into
jborean93:masterfrom
malthe:add-datetime-serialization

Conversation

@malthe
Copy link
Copy Markdown
Contributor

@malthe malthe commented Dec 13, 2021

This should be right according to docs:

image

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 13, 2021

Codecov Report

Merging #129 (6164d21) into master (dfc285d) will decrease coverage by 0.17%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   99.66%   99.48%   -0.18%     
==========================================
  Files          13       13              
  Lines        3297     3312      +15     
==========================================
+ Hits         3286     3295       +9     
- Misses         11       17       +6     
Flag Coverage Δ
py3.10 99.48% <66.66%> (-0.18%) ⬇️
py3.6 99.48% <66.66%> (-0.18%) ⬇️
py3.7 99.48% <66.66%> (-0.18%) ⬇️
py3.8 99.45% <66.66%> (-0.18%) ⬇️
py3.9 99.45% <66.66%> (-0.18%) ⬇️
ubuntu 98.21% <66.66%> (-0.18%) ⬇️
windows 99.48% <66.66%> (-0.19%) ⬇️
x64 99.48% <66.66%> (-0.18%) ⬇️
x86 99.48% <66.66%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pypsrp/serializer.py 98.87% <66.66%> (-1.13%) ⬇️
pypsrp/powershell.py 99.85% <0.00%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfc285d...6164d21. Read the comment docs.

@malthe malthe force-pushed the add-datetime-serialization branch from 0f78754 to ca73a81 Compare December 13, 2021 18:26
@malthe malthe force-pushed the add-datetime-serialization branch from 1adfd76 to 6164d21 Compare December 13, 2021 18:56
@jborean93
Copy link
Copy Markdown
Owner

I'm not sure this is something we can change at this point in time without breaking existing user's code. This alludes to the point I was making in #128 (comment)

The other benefit would be to make it easier to change these details with a new version without breaking backwards compatibility

Due to the nature of the serializer people have written code that expects types to come out in the same way as before. This means people who output a DateTime object expect a string in the ISO format. By changing it to be an actual datetime object it will most likely break their code. I unfortunately don't have an easy answer to this question and it's one of the reasons why the POC for the psrp namespace is completely separate (the serializer changes the objects it outputs).

From a technical perspective there's also the issue that the ISO format .NET outputs is precise up to 100s of nanoseconds whereas Python is only as precise a microsecond. This is effective 1 more digit on the fractional scaling causing issues when trying to parse it back in Python. For example in PowerShell you get:

pwsh -c "[System.Management.Automation.PSSerializer]::Serialize([DateTime]::Now)"

<Objs Version="1.1.0.1" xmlns="http://schemas.microsoft.com/powershell/2004/04">
  <DT>2021-12-14T04:57:05.5701415+10:00</DT>
</Objs>

In Python you get

python -c "import datetime; dt = datetime.datetime.now(); print(dt.replace(tzinfo=datetime.timezone.utc).isoformat())"

2021-12-14T04:58:42.621916+00:00

Note how .NET has 1 extra digit in the fractional second to denote 100s of nanoseconds. To be able to parse that in Python you either need to ignore the 7th digit (loosing precision) or somehow smuggle the nanoseconds value in the datetime object. You can see how psrpcore will serialize and deserialize a DateTime object.

Ultimately I'm not sure this can really be done without exposing some way for the caller to say I'm aware DateTime values become an actual datetime object in Python.

@malthe
Copy link
Copy Markdown
Contributor Author

malthe commented Dec 14, 2021

@jborean93, seems like a pretty thorough reworking of those serde routines, nice.

I suppose for me, backwards compatibility here I wouldn't be so concerned about given that we're still on sub-1.0 release numbers, but I think it would be relatively easy to make it optional.

We could introduce a new flag use_deprecated_datetime_handling which would allow reverting to the older behavior.

@jborean93
Copy link
Copy Markdown
Owner

I suppose for me, backwards compatibility here I wouldn't be so concerned about given that we're still on sub-1.0 release numbers, but I think it would be relatively easy to make it optional.

This is a valid point but my main use case is really for Ansible here and while this is sub-1.0 numbers I still need to ensure that I don't break anything there. This is one of the reasons why I'm moving all the new serializer and asyncio stuff to the psrp namespace.

I definitely need to think about this type of problem a bit more and figure out a good way to introduce breaking changes in the serializer as it will still be a problem in the new code. The biggest issue is that the serializer needs to work with both the library code and user facing code.

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.

2 participants