Skip to content

Add custom JSONEncoder for model serialization - #19595

Merged
Laia Patiño (laiapat) merged 11 commits into
Azure:mainfrom
laiapat:serialize-pr
Aug 4, 2021
Merged

Add custom JSONEncoder for model serialization#19595
Laia Patiño (laiapat) merged 11 commits into
Azure:mainfrom
laiapat:serialize-pr

Conversation

@laiapat

Copy link
Copy Markdown
Member

This adds a custom JSONEncoder that can serialize datetime objects (dates, times, datetimes, and timedeltas) in UTC ISO 8601 format, as well as bytes and bytearrays in base64 strings.

@laiapat Laia Patiño (laiapat) added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Jun 30, 2021
@laiapat
Laia Patiño (laiapat) force-pushed the serialize-pr branch 2 times, most recently from e4e71cd to 6ec7641 Compare July 8, 2021 01:04
@laiapat
Laia Patiño (laiapat) marked this pull request as ready for review July 8, 2021 01:59

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@xiangyan99

Copy link
Copy Markdown
Member

/azp run python - core - ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@xiangyan99

Copy link
Copy Markdown
Member

@mccoyp _utils.py has been moved into utils folder.

Comment thread sdk/core/azure-core/azure/core/serialization.py Outdated
Comment thread sdk/core/azure-core/azure/core/serialization.py Outdated
Comment thread sdk/core/azure-core/dev_requirements.txt Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When the conversation starts regarding deserialization - we will want to make sure we support weird .NET timestamps (with extra decimal places). When that conversation happens - what we do to serialize round-tripped values will be important as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need to import a private type from a private submodule?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should rename _FixedOffset to FixedOffset. But not sure if it is worth...

Comment thread sdk/core/azure-core/azure/core/serialization.py Outdated
@iscai-msft

Copy link
Copy Markdown
Member

/azp run python - autorest - pr

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this still need int() around seconds?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wouldn't think so, since this AttributeError should only come up if seconds is an integer. seconds.is_integer() works when seconds is a float on 2.7. 3.6, and 3.9 when I test locally

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is currently part of the public API - which I don't think it should be, at least to start with.
Can we make it private?

@annatisch

Copy link
Copy Markdown
Member

/azp run python - core - ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@laiapat

Copy link
Copy Markdown
Member Author

/azp run python - core - ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@laiapat

Copy link
Copy Markdown
Member Author

/azp run python - core - ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@laiapat
Laia Patiño (laiapat) enabled auto-merge (squash) August 4, 2021 01:33
@laiapat
Laia Patiño (laiapat) merged commit 6585a10 into Azure:main Aug 4, 2021
@laiapat
Laia Patiño (laiapat) deleted the serialize-pr branch August 4, 2021 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants