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

Add support for additional python types as dict keys in Struct.to_json #321

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Add support for additional python types as dict keys in Struct.to_json #321

merged 2 commits into from
Jul 17, 2024

Conversation

arhamchopra
Copy link
Collaborator

@arhamchopra arhamchopra commented Jul 11, 2024 •
edited
Loading

This PR adds support for more python types to be keys of a dictionary in the csp.Struct's to_json method.

Previously there was support for:

  • strings
  • integers
  • floats

With this PR, the following additional types will be handled:

  • None literal
  • bools
  • datetime, date, time (convert to ISO 8601 format)
  • csp.Enums (use the name attribute to convert to string)
  • Python enums (use the name attribute to convert to string)

@timkpaine
Copy link
Member

timkpaine commented Jul 11, 2024 •
edited
Loading

NOT the generic enums in python

Should we?

@timkpaine timkpaine added the type: enhancement Issues and PRs related to improvements to existing features label Jul 11, 2024
@arhamchopra
Copy link
Collaborator Author

arhamchopra commented Jul 12, 2024 •
edited
Loading

NOT the generic enums in python

Should we?

Would require us to import enums module in csp, as we would need to check if a python object is an enum type.
This is a bit different from datetime for 2 reasons. CPython has easy ways to import the datetime module and has direct APIs to check if a py object is of type datetime(https://github.com/python/cpython/blob/main/Include/datetime.h). Enums don't have any easily accessible API. In addition they won't be used anywhere other than to_json I think.
Lastly (more of a convention thing) there is a CSP standard to convert csp.Enums to strings. If we do want to support enums should we rely on the str method?

@arhamchopra arhamchopra marked this pull request as ready for review July 12, 2024 01:44
@timkpaine
Copy link
Member

It's possible to check if the type is a subclass of a python enum, let me see how ugly it is. On unhandled type, should we default to str?

@arhamchopra
Copy link
Collaborator Author

It's possible to check if the type is a subclass of a python enum, let me see how ugly it is. On unhandled type, should we default to str?

Yes, it should be.
PyImport_ImportModule("enum") // since Python 3 has enums in the standard library
"Extract enum.Enum PyTypeObject" // Not very sure how to do this since we don't have a Python C API here
if( PyType(py_obj) == EnumType ) // Compare if current py_obj type is a subtype of base enum type.

@arhamchopra arhamchopra force-pushed the ac/add_more_python_types_as_dict_keys_struct_to_json branch from 775cf61 to 3332483 Compare July 16, 2024 18:18
@arhamchopra
Copy link
Collaborator Author

It's possible to check if the type is a subclass of a python enum, let me see how ugly it is. On unhandled type, should we default to str?

Added support for python enums in the latest push

AdamGlustein
AdamGlustein requested changes Jul 17, 2024
View reviewed changes
cpp/csp/python/PyStructToJson.cpp Outdated Show resolved Hide resolved
cpp/csp/python/PyStructToJson.cpp Outdated Show resolved Hide resolved
cpp/csp/python/PyStructToJson.cpp Show resolved Hide resolved
cpp/csp/python/PyStructToJson.cpp Outdated Show resolved Hide resolved
cpp/csp/python/PyStructToJson.cpp Outdated Show resolved Hide resolved
cpp/csp/python/PyStructToJson.cpp Outdated Show resolved Hide resolved
csp/tests/impl/test_struct.py Show resolved Hide resolved
AdamGlustein
AdamGlustein requested changes Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@AdamGlustein AdamGlustein left a comment

Choose a reason for hiding this comment

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

Fix typo in docs

docs/wiki/api-references/csp.Struct-API.md Outdated Show resolved Hide resolved
@arhamchopra arhamchopra force-pushed the ac/add_more_python_types_as_dict_keys_struct_to_json branch from a7db187 to 92be596 Compare July 17, 2024 14:23
@arhamchopra arhamchopra merged commit 4b7c37a into main Jul 17, 2024
30 checks passed
@arhamchopra arhamchopra deleted the ac/add_more_python_types_as_dict_keys_struct_to_json branch July 17, 2024 15:57
wrieg123 pushed a commit to wrieg123/csp that referenced this pull request Jul 20, 2024
@arhamchopra @wrieg123
Add support for additional python types as dict keys in Struct.to_json (
4aca52b 
Point72#321)

* Add support for additional python types as dict keys in Struct.to_json

Signed-off-by: Arham Chopra <[email protected]>

* Fix memory leaks, add docs, and address comments

Signed-off-by: Arham Chopra <[email protected]>

---------

Signed-off-by: Arham Chopra <[email protected]>
robambalu
robambalu reviewed Jul 22, 2024
View reviewed changes
cpp/csp/python/PyStructToJson.cpp
const char * str = PyUnicode_AsUTF8AndSize( str_obj.get(), &len );
val.SetString( str, len, doc.GetAllocator() );
}
else if( PyUnicode_Check( py_key ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

might want to check string first since that is by far the most common

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timkpaine timkpaine timkpaine left review comments

@robambalu robambalu robambalu left review comments

@AdamGlustein AdamGlustein AdamGlustein approved these changes

@svatasoiu svatasoiu Awaiting requested review from svatasoiu svatasoiu is a code owner

@alexddobkin alexddobkin Awaiting requested review from alexddobkin alexddobkin is a code owner

Assignees
No one assigned
Labels
type: enhancement Issues and PRs related to improvements to existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arhamchopra @timkpaine @robambalu @AdamGlustein