Add serde support by daboross · Pull Request #55 · bluss/arrayvec

daboross · 2017-06-08T08:35:52Z

This implements serde support under the optional 'serde' feature, and adds unit tests to test said support.

Serde will be optionally included for runtime, but is unfortunately required for running tests - as cargo does not have support for optional dev-dependencies. FEATURES="serde" is added to the travis config, as serde-support tests will not run without it.

Fixes#54.

…rayString. This implements serde support under the optional 'serde' feature, and adds unit tests to test said support. https://serde.rs/unit-testing.html used as a guide for the unit tests - using 'serde_test' makes for much less boilerplate here, but it does require that the project have a non-optional dev dependency on 'serde_test'.

daboross · 2017-06-08T08:43:55Z

Sorry about that - removed usages of '?' in the code.

daboross · 2017-06-08T09:32:25Z

@bluss Should I re-make this PR towards the 0.3 branch instead?

bluss · 2017-06-08T18:41:39Z

Hm probably not. We should aim forwards.

I'm thinking here I'd prefer if the feature flag was versioned and had a separate name from the crate. So a feature name like serde-1 or use_serde would sit better. I guess the standard is just "serde" but I've seen crates where that is a problem.

daboross · 2017-06-08T18:44:16Z

@bluss Ah, just missed the comment! I had just filed a PR for the 0.3 branch, but it seems it fails anyways with the rust 1.2.0 support.

I was basing the serde name off of https://github.com/brson/rust-api-guidelines/blob/master/README.md#data-structures-implement-serdes-serialize-deserialize-c-serde, which states that the feature should be the crate name itself.

edit: Correction, what I meant to link was https://github.com/brson/rust-api-guidelines/blob/master/README.md#crate-has-a-serde-cfg-option-that-enables-serde-c-serde-cfg, right below what I linked above.

bluss · 2017-06-08T18:49:21Z

Thanks for the link, I don't think it's that simple, even though it's a neat solution for that particular issue. It's time to break the feature and optional crate namespace merge. Using the crate name limits options when it comes to the evolution of the crate and avoiding breaking changes.

bluss · 2017-06-08T18:51:07Z

I for example think that ndarray 0.9, which supports serde 0.9, it could have added support for serde 1.0 without breaking change if it had the foresight.

daboross · 2017-06-08T18:54:44Z

Ah, that makes sense. I wasn't sure if depending on multiple versions of a crate would be possible even under different optional features. serde-1 is good then?

bluss · 2017-06-08T18:58:41Z

Right -- it's not straightforward to do such a dependency. But the start of it exists 😄 as legacy-serde. Maybe the idea is not fully matured.

I think serde-1 is the way to go.

daboross · 2017-06-08T19:06:47Z

Sounds good then! Feature name updated.

dtolnay · 2017-06-11T19:17:41Z

If you are versioning the feature to possibly support multiple Serde versions in the same arrayvec in the future, I would prefer to standardize on the official shims. Add a dependency on serde1 and implement serde1::{Serialize, Deserialize}. That way no implicit "serde" feature is inferred by Cargo.

dtolnay · 2017-06-11T19:22:04Z

+ type Value = ArrayString<A>;
+
+ fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+ write!(formatter, "a string with no more than {} elements", A::capacity())


The word "element" of a string is unclear to me. Is this terminology used elsewhere in arrayvec? I would prefer to say "a string of no more than {} bytes".

Sure, that makes more sense.

daboross · 2017-06-15T05:00:19Z

Thank you for the comment about the messages @dtolnay, I think the string message was really just copy-pasted from the ArrayVec one. Should be more clear now!

daboross · 2017-06-15T05:01:19Z

@bluss thoughts on depending on serde1 vs. serde under a serde-1 feature?

Edit: I mean I'd be OK with either, and I can update this PR to whichever you'd choose for the project.

Thanks to @camlorn for the idea.

daboross · 2017-06-16T22:19:07Z

Just added failure tests, thanks to #58 for the idea.

P.S. Sorry, ignore my last comment - I just had a bug in the test code I wrote.

alexbool · 2017-07-13T07:49:32Z

@bluss friendly ping

bluss · 2017-07-30T10:08:35Z

Well, I'm certainly being unfriendly by not being on top of this. (Not guaranteed to be here at any one particular month of the year, but some of them would be nice...) Let's see..

bluss · 2017-07-30T10:10:49Z

Comaintainers / maintainers that are interested in taking care of this crate? I'm not planning on stepping down, but I clearly don't have enough time to do the whole job.

bluss · 2017-07-30T10:15:43Z

Many thanks for the PR

bluss · 2017-07-31T17:54:54Z

Features are in a deadlock in arrayvec, since the 0.3 branch says the version requirement is Rust 1.2.0. Ridiculous, sure. So we can't release any new features on that branch. (Master already tests from newer versions in travis.)

Hoping to resolve all the planned API changes for the next version and release 0.4. It's not ideal, but the issues are pretty long standing as well.

daboross mentioned this pull request Jun 8, 2017
Add serde support to 0.3 #57
Closed

Change feature name from 'serde' to 'serde-1'.
1b0168b

ahicks92 mentioned this pull request Jun 11, 2017
Add serde support. #58
Closed

dtolnay reviewed Jun 11, 2017
View reviewed changes

Remove ambiguities in serde expecting messages.
704f237

Add failure tests for serde implementations
6cd03dc
Thanks to @camlorn for the idea.

bluss merged commit 2949720 into bluss:masterJul 30, 2017

bluss mentioned this pull request Jul 30, 2017
Prepare 0.4, more vec-like api and error reform #61
Merged

daboross deleted the serde branch August 9, 2017 21:07

mathstuf mentioned this pull request Jan 11, 2021
[Tracking] Rust 1.48 introduced crashes for a project; 0.4.11 works. Unsure of the reason #173
Closed

Conversation

daboross commented Jun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daboross commented Jun 8, 2017

Uh oh!

daboross commented Jun 8, 2017

Uh oh!

bluss commented Jun 8, 2017

Uh oh!

daboross commented Jun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss commented Jun 8, 2017

Uh oh!

bluss commented Jun 8, 2017

Uh oh!

daboross commented Jun 8, 2017

Uh oh!

bluss commented Jun 8, 2017

Uh oh!

daboross commented Jun 8, 2017

Uh oh!

dtolnay commented Jun 11, 2017

Uh oh!

dtolnayJun 11, 2017

Choose a reason for hiding this comment

Uh oh!

daborossJun 11, 2017

Choose a reason for hiding this comment

Uh oh!

daboross commented Jun 15, 2017

Uh oh!

daboross commented Jun 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daboross commented Jun 16, 2017

Uh oh!

alexbool commented Jul 13, 2017

Uh oh!

bluss commented Jul 30, 2017

Uh oh!

bluss commented Jul 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss commented Jul 30, 2017

Uh oh!

bluss commented Jul 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daboross commented Jun 8, 2017•
edited
Loading

daboross commented Jun 8, 2017•
edited
Loading

daboross commented Jun 15, 2017•
edited
Loading

bluss commented Jul 30, 2017•
edited
Loading