Skip to content

Support seq and maps (rebase of #142, refactor of #129) #154

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

Closed
wants to merge 6 commits into from

Conversation

joemat
Copy link

@joemat joemat commented Apr 12, 2021

Based on #142 and #129 this PR adds support to serialize maps and vectors.
See description of #142 for details.

This is just a rebased version of #142 which fixes the merge conflict.

Kudos to @adrianbenavides and @Niedzwiedzw for their work on this.

@ghost
Copy link

ghost commented Apr 12, 2021

Closes #135.

@ghost
Copy link

ghost commented Apr 12, 2021

FYI, I've contributed to and find this project useful and so proposed we move it to a group: #155.

}

#[test]
fn test_serialize_map_within_struct() {
Copy link

Choose a reason for hiding this comment

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

This test actually fails intermittently.

I'm wondering if it is from the implementation of serializing Map's or the test relying on a fixed order of printed keys.

Choose a reason for hiding this comment

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

Exactly, this test is relying on the Map's items being ordered, which is totally wrong. Sorry for that.

I'd suggest removing the second insert or relaxing the assert using .contains() to assert that at least the content is ok, rather than the exact output content+order.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the hints, I pushed an update to fix the test.

Copy link

Choose a reason for hiding this comment

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

Exactly, this test is relying on the Map's items being ordered, which is totally wrong. Sorry for that.

ah, makes sense @adrianbenavides. Thank you for explaining! No need to apologize too. Part of learning!

Copy link

Choose a reason for hiding this comment

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

stellar, thanks @joemat!

Copy link

Choose a reason for hiding this comment

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

Huh, cannot find the resolve button. I ran cargo test ser repeatedly locally and things look great!

@XiaoXiaoSN
Copy link

Hi, I noticed this awesome job.
But it seems to have been around for a while and has some conflicts from the master branch.

Would you complete this Pull Request? Or if possible I can also take over and resolve the conflicting part.

@victorpaleologue
Copy link

Hi, I noticed this awesome job. But it seems to have been around for a while and has some conflicts from the master branch.

Would you complete this Pull Request? Or if possible I can also take over and resolve the conflicting part.

FTR you did the rebase here #170, thanks.

@wt
Copy link

wt commented Jan 9, 2023

Should this be closed because of #173? #170 says that #173 merges the changes from some other changes including this one.

@joemat
Copy link
Author

joemat commented Jan 22, 2023

yes, makes sense to close this one then

@joemat joemat closed this Jan 22, 2023
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.

5 participants