fix setting root element and don't return empty values #28
Conversation
olafura
requested review from
MIchaelMainer,
Ndiritu,
andrueastman,
baywet,
calebkiage,
peombwa,
rkodev and
zengin
as code owners
baywet
left a comment
baywet
left a comment
There was a problem hiding this comment.
thanks for the contribution.
Can you also add an entry to the changelog and bump the patch version please?
(see review in abstractions)
baywet
left a comment
baywet
left a comment
There was a problem hiding this comment.
Thanks for making the changes!
|
@olafura the unit tests seem to be failing with this change, can you look into it please? |
olafura
force-pushed
the
olafura/fix-serializer-writer
branch
from
e98d645 to
aa69aa1
Compare
|
|
@microsoft-github-policy-service agree |
| end | ||
| end | ||
|
|
||
| def write_object_value(key, value) |
There was a problem hiding this comment.
sorry, I don't think we can accept this pull request as is.
This change effectively modifies the interface (updated needed in abstractions and other implementations) and would require changes in the generator.
The main problem with that approach is that it diverges from other languages
Can you create an issue where you provide more context about what you are doing and what's not working?
(snippet code for the request, stack trace if any, sent and received payload, etc...)
There was a problem hiding this comment.
The problem is here and can also be addressed there if more appropriate:
https://github.com/microsoft/kiota-abstractions-ruby/blob/main/lib/microsoft_kiota_abstractions/request_information.rb#L99-L101
Since with both objects and arrays we don't modify the writer only return a temp version. We don't actually set anything there.
I was going by the assumption that if you pass nil as the first param you want to add the value to the root. If that is the wrong assumption the I can move my PR to kiota-abstractions-ruby
There was a problem hiding this comment.
the assumption is correct.
I think the issue is that having any of the "write" methods returning a new writer. This was a temporary workaround done by the intern years ago so his demo could work at the time, but it diverges from the other language implementations and this is what we should look to correct here.
|
This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged. |
2 participants
When setting the root we don't seem to be setting it to the writer so when we call get_serialized_content we don't get anything.
Also the ms graph api doesn't like empty values so I clean them up before returning.