[WIP]Refactor AzureJSONEncoder

[Codejune]

Relation of #17102

Descriptions

• Added _timestr_as_isostr() method for reducing indent
• Remove duplicate method calls try super().default()

[ghost]

Thank you for your contribution Codejune! We will review the pull request and get back to you soon.

[mccoyp]

Thank you so much for opening a PR for this! I just have a few suggestions, but this is a great refactoring

[Codejune]

@mccoyp @xiangyan99
I've confirmed that in Azure Pipelines/python-autorest-pr I'm still getting the error.
It still happened even after I updated the commit from the upstream/main branch.
Am I missing something in this step?

Pipelines/python-autorest-pr failures log

[xiangyan99]

/azp run python - autorest - pr

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mccoyp]

This looks great! Just a few small comments. We're also working on resolving the issue in the python - autorest - pr pipeline -- I'll give an update when that's fixed so we can merge this

[xiangyan99]

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"

[mccoyp]

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:

[Codejune]

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.

[xiangyan99]

typing.NewType is introduced in python 3, right?

[Codejune]

Yes. It was added in Python 3.

[xiangyan99]

Unfortunately, we need to support 2.7 as well.

[Codejune]

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. :)

[Codejune]

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?

[xiangyan99]

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?

[Codejune]

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