Skip to content

Conversation

corenting
Copy link

Description

The aim of this PR is to have support for ipaddress objects in JSONEncoder, similar to what was done for UUIDs for example.

@auvipy auvipy self-requested a review August 27, 2023 14:45
@corenting corenting force-pushed the jsonencoder_ipaddress branch from 277e04a to ef0ad04 Compare October 29, 2023 17:39
@auvipy auvipy requested a review from a team October 30, 2023 06:41
@corenting
Copy link
Author

@auvipy sorry for the ping, is it possible to take a look?

@auvipy
Copy link
Collaborator

auvipy commented Jan 11, 2024

Yes of course

@math-a3k math-a3k mentioned this pull request Jan 24, 2024
Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 17, 2024
@corenting corenting force-pushed the jsonencoder_ipaddress branch from ef0ad04 to afdcf5c Compare March 17, 2024 19:22
@stale stale bot removed the stale label Mar 17, 2024
Copy link
Contributor

@tfranzel tfranzel left a comment

Choose a reason for hiding this comment

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

I believe this should be very safe to add and has afais no potential for a regression.

ipaddress was added in python 3.3, and thus should be safe to import too.

I think this is a good change because it closes a functional gap, that one would consider to not even be there, until it bites you.

Copy link

stale bot commented Apr 28, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 28, 2025
Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

should we also document this?

@auvipy auvipy removed the stale label Apr 28, 2025
Copy link

stale bot commented Jul 19, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@corenting corenting closed this Sep 19, 2025
@corenting corenting reopened this Sep 19, 2025
@corenting
Copy link
Author

corenting commented Sep 19, 2025

should we also document this?

Sorry I hadn't seen the comment (and for the closed/reopened issue, I missclicked). Should I add it somewhere in the doc, or maybe in a separate PR to first merge this one ?

@browniebroke
Copy link
Collaborator

should we also document this?

Good question.

The only relevant piece of documentation I could find is this page. Currently, it stays relatively high level and doesn't try to document how each data type is rendered, which I think is fine.

So unless you know of an existing place where the doc is more detailed on JSON encoding, I would suggest to not bother with adding it.

@corenting
Copy link
Author

should we also document this?

Good question.

The only relevant piece of documentation I could find is this page. Currently, it stays relatively high level and doesn't try to document how each data type is rendered, which I think is fine.

So unless you know of an existing place where the doc is more detailed on JSON encoding, I would suggest to not bother with adding it.

Thanks for checking! I will leave the MR as-is then.

Copy link
Collaborator

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

Apologies for not picking this up earlier, would you mind maing these changes? Looks good otherwise

@browniebroke browniebroke added this to the 3.17 milestone Sep 19, 2025
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.

4 participants