Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

[issue-511] Fix serialization of additional data on Collections#512

Merged
baywet merged 5 commits intomicrosoftgraph:devmicrosoftgraph/msgraph-sdk-java:devfrom
bougar:bugfix/511bougar/msgraph-sdk-java:bugfix/511Copy head branch name to clipboard
Oct 2, 2020
Merged

[issue-511] Fix serialization of additional data on Collections#512
baywet merged 5 commits intomicrosoftgraph:devmicrosoftgraph/msgraph-sdk-java:devfrom
bougar:bugfix/511bougar/msgraph-sdk-java:bugfix/511Copy head branch name to clipboard

Conversation

@bougar
Copy link

@bougar bougar commented Oct 1, 2020

This PR tries to solve #511 issue.
I just modified the getChildAdditionalData method in order to support the serialization of additional properties on List items.

@ghost
Copy link

ghost commented Oct 1, 2020

CLA assistant check
All CLA requirements met.

@baywet baywet self-requested a review October 1, 2020 11:28
@baywet baywet self-assigned this Oct 1, 2020
@baywet baywet added this to the 2.3.0 milestone Oct 1, 2020
@baywet baywet linked an issue Oct 1, 2020 that may be closed by this pull request
@baywet
Copy link
Member

baywet commented Oct 1, 2020

@bougar thanks for the PR, I'll review it shortly. In the meantime, can you make sure you sign the CLA please? (see previous message)

baywet added 2 commits October 1, 2020 15:28
- fixes a bug where additional data would be added to the wrong level for lists
baywet
baywet previously approved these changes Oct 1, 2020
@baywet
Copy link
Member

baywet commented Oct 1, 2020

thanks @bougar for the pull request. I made a few modifications:

  • the unit test you added was testing presence of data but not that the data was "at the right place" (we have to revamp our unit tests for that aspect as well...)
  • there was a bug were the serialized data was not being added at the right level
  • I did some refactoring at the method had a lot of duplicated code and was hard to read
    Great job on catching the issue and starting the work to fix it!

@baywet
Copy link
Member

baywet commented Oct 1, 2020

BTW @bougar if you sign up here and send a few other pull requests within the month, they'll send you a cool t-shirt!
https://hacktoberfest.digitalocean.com/

Copy link
Collaborator

@MIchaelMainer MIchaelMainer left a comment

Choose a reason for hiding this comment

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

Reviewed via Teams. LGTM

@baywet baywet merged commit 8c3cb95 into microsoftgraph:dev Oct 2, 2020
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.

AdditionalData not serialized for collections

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.