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.

Sign up for GitHub

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

Jump to bottom

[WIP]Refactor AzureJSONEncoder #21028

Merged
merged 5 commits into from
Oct 18, 2021
Merged

[WIP]Refactor AzureJSONEncoder #21028

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fb50ae9
Refactor AzureJSONEncoder
Codejune Oct 3, 2021
e68b8d8
Improved readability of AzureJSONEncoder datetime handling
Codejune Oct 9, 2021
efe1af8
Add some docstring
Codejune Oct 9, 2021
76fc86e
Fix typo and add type checking
Codejune Oct 14, 2021
065e4f2
Improve type checking and rename some inner variables
Codejune Oct 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 41 additions & 39 deletions sdk/core/azure-core/azure/core/serialization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@
# --------------------------------------------------------------------------
import base64
from json import JSONEncoder
from typing import TYPE_CHECKING

from typing import Union, cast
from datetime import datetime, date, time, timedelta
from .utils._utils import _FixedOffset

if TYPE_CHECKING:
from datetime import timedelta

__all__ = ["NULL", "AzureJSONEncoder"]

Expand All @@ -32,15 +30,15 @@ def __bool__(self):
"""


def _timedelta_as_isostr(value):
def _timedelta_as_isostr(td):
# type: (timedelta) -> str
"""Converts a datetime.timedelta object into an ISO 8601 formatted string, e.g. 'P4DT12H30M05S'

Function adapted from the Tin Can Python project: https://github.com/RusticiSoftware/TinCanPython
"""

# Split seconds to larger units
seconds = value.total_seconds()
seconds = td.total_seconds()
minutes, seconds = divmod(seconds, 60)
hours, minutes = divmod(minutes, 60)
days, hours = divmod(hours, 24)
Expand All @@ -49,22 +47,22 @@ def _timedelta_as_isostr(value):
seconds = round(seconds, 6)

# Build date
date = ""
date_str = ""
if days:
date = "%sD" % days
date_str = "%sD" % days

# Build time
time = "T"
time_str = "T"

# Hours
bigger_exists = date or hours
bigger_exists = date_str or hours
if bigger_exists:
time += "{:02}H".format(hours)
time_str += "{:02}H".format(hours)

# Minutes
bigger_exists = bigger_exists or minutes
if bigger_exists:
time += "{:02}M".format(minutes)
time_str += "{:02}M".format(minutes)

# Seconds
try:
Expand All @@ -78,9 +76,32 @@ def _timedelta_as_isostr(value):
except AttributeError: # int.is_integer() raises
seconds_string = "{:02}".format(seconds)

time += "{}S".format(seconds_string)
time_str += "{}S".format(seconds_string)

return "P" + date_str + time_str


return "P" + date + time
def _datetime_as_isostr(dt):
# type: (Union[datetime, date, time, timedelta]) -> str
"""Converts a datetime.(datetime|date|time|timedelta) object into an ISO 8601 formatted string"""
# First try datetime.datetime
if hasattr(dt, "year") and hasattr(dt, "hour"):
dt = cast(datetime, dt)
# astimezone() fails for naive times in Python 2.7, so make make sure dt is aware (tzinfo is set)
if not dt.tzinfo:
Copy link
Member

Choose a reason for hiding this comment

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

Mypy error:

error: Item "date" of "Union[datetime, date, time, timedelta]" has no attribute "tzinfo"
error: Item "timedelta" of "Union[datetime, date, time, timedelta]" has no attribute "tzinfo"
error: Item "timedelta" of "Union[datetime, date, time, timedelta]" has no attribute "replace"

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering what the best way to resolve these would be... maybe casting to the type we know it should be at each point? e.g. in this case,

if not cast(datetime, dt).tzinfo:

Copy link
Contributor Author

@Codejune Codejune Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should check using typing.Newtype() for datetime, date, time, and timedelta. Something like

from typing import TYPE_CHECKING, Union, cast
...
if TYPE_CHECKING:
    from datetime import datetime, date, time, timedelta
    datetime_type = typing.NewType('datetime_type', datetime)
    date_type = typing.NewType('datetime_type', date)
    time_type = typing.NewType('datetime_type', time)
    timedelta_type = typing.NewType('datetime_type', timedelta)
 ...
def _datetime_as_isostr(dt):
    # type: (Union[datetime, date, time, timedelta]) -> str
    # First try datetime.datetime
    if hasattr(dt, "year") and hasattr(dt, "hour"):
        dt = cast(datetime_type, dt)
        if not dt.tzinfo:
            iso_formatted = dt.replace(tzinfo=TZ_UTC).isoformat()
        else:
            iso_formatted = dt.astimezone(TZ_UTC).isoformat()
        return iso_formatted.replace("+00:00", "Z")
    # Next try datetime.date or datetime.time
    try:
        dt = cast(Union[date_type, time_type], dt)
        return dt.isoformat()
    # Last, try datetime.timedelta
    except AttributeError:
        dt = cast(timedelta_type, dt)
        return _timedelta_as_isostr(dt)

This method passed tox -e mypy -c ../../../eng/tox/tox.ini .

mypy run-test: commands[0] | /Users/codejune/Develop/azure-sdk-for-python/sdk/core/azure-core/.tox/mypy/bin/python /Users/codejune/Develop/azure-sdk-for-python/sdk/core/azure-core/../../../eng/tox/run_mypy.py -t /Users/codejune/Develop/azure-sdk-for-python/sdk/core/azure-core
INFO:root:Package azure-core has opted to run mypy
Success: no issues found in 71 source files
_________________________________________________________________________________________________ summary _________________________________________________________________________________________________
  mypy: commands succeeded
  congratulations :)

But it looks like hardcoded. So I'm thinking of a better way to do this.

Copy link
Member

Choose a reason for hiding this comment

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

typing.NewType is introduced in python 3, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It was added in Python 3.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we need to support 2.7 as well.

Copy link
Contributor Author

@Codejune Codejune Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. By previous comments I have know that the sdk should support python 2.7.
So I didn't committed this method yet. But your suggestions are helping me a lot. Thank you. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about not using typing.NewType? As follows.

from typing import Union, cast
# if TYPE_CHECKING:
from datetime import datetime, date, time, timedelta
...
def _datetime_as_isostr(dt):
    # type: (Union[datetime, date, time, timedelta]) -> str
    # First try datetime.datetime
    if hasattr(dt, "year") and hasattr(dt, "hour"):
        dt = cast(datetime, dt)
        ...
    # Next try datetime.date or datetime.time
    try:
        dt = cast(Union[date, time], dt)
        ...
    # Last, try datetime.timedelta
    except AttributeError:
        dt = cast(timedelta, dt)
        ...

It passed all my local tox tests.
However, I haven't tested it against Python 2.7 yet.
And I wonder if there are side effects caused by removing the if TYPE_CHECKING: check.
So I need to test it with Azure Pipeline.
How do you think of this?

Copy link
Member

Choose a reason for hiding this comment

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

It is too much for a developer to test all the combinations, so we setup the pipeline. :)

Feel free to leverage the pipeline to test py2.7.

We don't encourage removing type checking because sometimes it does find real issues. Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! :)
I'll add a commit on this soon and check it out.
Thank you for your time!

iso_formatted = dt.replace(tzinfo=TZ_UTC).isoformat()
Codejune marked this conversation as resolved.
Show resolved Hide resolved
else:
iso_formatted = dt.astimezone(TZ_UTC).isoformat()
# Replace the trailing "+00:00" UTC offset with "Z" (RFC 3339: https://www.ietf.org/rfc/rfc3339.txt)
return iso_formatted.replace("+00:00", "Z")
# Next try datetime.date or datetime.time
try:
dt = cast(Union[date, time], dt)
return dt.isoformat()
Codejune marked this conversation as resolved.
Show resolved Hide resolved
# Last, try datetime.timedelta
except AttributeError:
dt = cast(timedelta, dt)
return _timedelta_as_isostr(dt)
Codejune marked this conversation as resolved.
Show resolved Hide resolved


try:
Expand All @@ -95,29 +116,10 @@ class AzureJSONEncoder(JSONEncoder):
"""A JSON encoder that's capable of serializing datetime objects and bytes."""

def default(self, o): # pylint: disable=too-many-return-statements
if isinstance(o, (bytes, bytearray)):
Codejune marked this conversation as resolved.
Show resolved Hide resolved
return base64.b64encode(o).decode()
try:
Codejune marked this conversation as resolved.
Show resolved Hide resolved
return super(AzureJSONEncoder, self).default(o)
except TypeError:
if isinstance(o, (bytes, bytearray)):
return base64.b64encode(o).decode()
try:
# First try datetime.datetime
if hasattr(o, "year") and hasattr(o, "hour"):
# astimezone() fails for naive times in Python 2.7, so make make sure o is aware (tzinfo is set)
if not o.tzinfo:
iso_formatted = o.replace(tzinfo=TZ_UTC).isoformat()
else:
iso_formatted = o.astimezone(TZ_UTC).isoformat()
# Replace the trailing "+00:00" UTC offset with "Z" (RFC 3339: https://www.ietf.org/rfc/rfc3339.txt)
return iso_formatted.replace("+00:00", "Z")
# Next try datetime.date or datetime.time
return o.isoformat()
except AttributeError:
pass
# Last, try datetime.timedelta
try:
return _timedelta_as_isostr(o)
except AttributeError:
# This will be raised when it hits value.total_seconds in the method above
pass
return super(AzureJSONEncoder, self).default(o)
return _datetime_as_isostr(o)
except AttributeError:
pass
return super(AzureJSONEncoder, self).default(o)