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.

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

Create Types for untyped nodes #286

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rohitkrsoni
Copy link

Overview

This PR adds Untyped nodes classes to support microsoft/kiota#4385

Related Issue

Fixes #285

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

Thanks for starting this @rohitkrsoni

Just a couple of comments here. Will you also look into writing the serialization implementation as well?

@@ -0,0 +1,24 @@
from .untyped_node import UntypedNode

class UntypedDictionary(UntypedNode):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class UntypedDictionary(UntypedNode):
class UntypedObject(UntypedNode):

Is there any reason to not call it UntypedObject?

Copy link
Author

@rohitkrsoni rohitkrsoni Jun 3, 2024

Choose a reason for hiding this comment

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

Initially, I though of adding it as UntypedObject but in python we call it dictionary which stores key-value pairs similar to HashMap in Java. https://docs.python.org/3/tutorial/datastructures.html#dictionaries

I can change it back to Object if you like.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I don't have strong opinion on this one. It's better to align with what the language thinks is best.
Maybe @shemogumbe can weigh in on the naming here..

Copy link
Author

Choose a reason for hiding this comment

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

My view is - arrays and objects instead of list and dict might create confusion as the codebase of the project using python client will have lists and dictionaries.

open for suggestions.

Copy link
Member

@andrueastman andrueastman Aug 26, 2024

Choose a reason for hiding this comment

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

@rohitkrsoni Lets keep this as is for now since the it makes sense for the language to have the naming as so and then start working on the serialization bits. In the event of any feedback before we merge, we can do a renaming.

Are you able to work on the serialization bits with this branch as a source?

kiota_abstractions/serialization/untyped_dict.py Outdated Show resolved Hide resolved
kiota_abstractions/serialization/untyped_list.py Outdated Show resolved Hide resolved
@rohitkrsoni
Copy link
Author

Thanks for starting this @rohitkrsoni

Just a couple of comments here. Will you also look into writing the serialization implementation as well?

Yes I will look into serialization implementation as well, but first I need to get my head around it.

@andrueastman
Copy link
Member

Yes I will look into serialization implementation as well, but first I need to get my head around it.

No worries. It would be great if we can get that started as well before we merge this one to avoid any unwanted regressions.

@rohitkrsoni
Copy link
Author

Yes I will look into serialization implementation as well, but first I need to get my head around it.

No worries. It would be great if we can get that started as well before we merge this one to avoid any unwanted regressions.

Sure, will start that as well. I'm afraid we might need to put this on hold for some time.

Copy link

sonarcloud bot commented Jun 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@baywet
Copy link
Member

baywet commented Aug 23, 2024

@rohitkrsoni @andrueastman just noticing this was a bit forgotten about. How can we get this finalized? (who's expecting what at this point?)

@rohitkrsoni
Copy link
Author

@baywet I was waiting for a confirmation on weather to define it as UntypedDictionary and UntypedLists or UntypedObject and UntypedArrays and then it needs review

#286 (comment)

After that we need to work on serialization ticket.

Thanks

Copy link
Contributor

This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create types for untyped nodes
3 participants