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

Serialization support #79

Merged
merged 19 commits into from
Apr 4, 2020
Merged

Serialization support #79

merged 19 commits into from
Apr 4, 2020

Conversation

GarkGarcia
Copy link
Contributor

@GarkGarcia GarkGarcia commented Mar 27, 2020

This is a follow up to #15.


This change is Reviewable

Cargo.toml Outdated Show resolved Hide resolved
Unfurtunately, a phantom `HashMapVisitor` struct had to be created.
src/map.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
src/set.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
…ocating the output by using `HashMap::with_capacity_and_hasher` and `HashMap::with_hasher` instead of `HashMap::default`
@GarkGarcia
Copy link
Contributor Author

GarkGarcia commented Mar 29, 2020

I'm not sure what I should do about the Minimum supported Rust version: 1.36.0 thing (which is causing CI to block)...

@jonhoo
Copy link
Owner

jonhoo commented Mar 30, 2020

Well, it fails because of the use of the todo! macro, which seems to be easy to just fill in? I think "a map" and "a set" is the style of message that should go in there (again, see hashbrown).

@GarkGarcia
Copy link
Contributor Author

Well, it fails because of the use of the todo! macro, which seems to be easy to just fill in? I think "a map" and "a set" is the style of message that should go in there (again, see hashbrown).

I see. I didn't know what was causing the block. I'll fix the as soon as I can.

@jonhoo
Copy link
Owner

jonhoo commented Mar 30, 2020

You can see the build errors by clicking "details" next to the failed test, then "View more details on Azure Pipelines" at the bottom of the shown pane, and then clicking the failing build step in the list on the left.

My bad, I forgot pass the `--features serde` flag when testing..
src/set.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
src/set.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Mar 31, 2020

Okay, one last question before we merge this: do we want to separate the impls of external traits (like serde) into a separate file the way hashbrown does? Might help avoid polluting map.rs and set.rs over time?

@jonhoo
Copy link
Owner

jonhoo commented Apr 1, 2020

Oh, and we should probably add some rudimentary tests here. A round-trip test for each data structure should be sufficient (that is, that deserialize(x.serialize()) == x).

@GarkGarcia
Copy link
Contributor Author

Okay, one last question before we merge this: do we want to separate the impls of external traits (like serde) into a separate file the way hashbrown does? Might help avoid polluting map.rs and set.rs over time?

Makes sense to me. I'll work on that.

@GarkGarcia
Copy link
Contributor Author

GarkGarcia commented Apr 2, 2020

Oh, and we should probably add some rudimentary tests here. A round-trip test for each data structure should be sufficient (that is, that deserialize(x.serialize()) == x).

That would be nice. Is there a way I can import a crate only in the context of test though? I'd have to use something like serde_json, but we should have it as a dependency (it should be a dev-dependency).

@jonhoo
Copy link
Owner

jonhoo commented Apr 2, 2020

I'm not sure I follow? Just add it as a dev-dependency, and you should be fine.

@GarkGarcia
Copy link
Contributor Author

I'm not sure I follow? Just add it as a dev-dependency, and you should be fine.

Yes, I just don't know how to do that hehe.

@GarkGarcia
Copy link
Contributor Author

GarkGarcia commented Apr 2, 2020

I've pushed a commit to GarkGarcia:master, but I doesn't show up in here for some reason...

@jonhoo
Copy link
Owner

jonhoo commented Apr 2, 2020

GitHub is having issues :)

To add a test dependency, just list it under here:

[dev-dependencies]

@GarkGarcia
Copy link
Contributor Author

To add a test dependency, just list it under here:

[dev-dependencies]

Thanks!

GitHub is having issues :)

Ohh I see. I found it strange that my commit was rejected a couple of times due to Internal Server Issues.

@GarkGarcia
Copy link
Contributor Author

All done (?)

@jonhoo jonhoo merged commit 7a27f38 into jonhoo:master Apr 4, 2020
@jonhoo
Copy link
Owner

jonhoo commented Apr 4, 2020

Nice, thank you!

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.

2 participants