For the moment I have introduced BareItem::Validated* variants adjacent to their unvalidated enum member counterparts.

Do you see us keeping both Validated and unvalidated enum types, or do you intend to remove the unvalidated ones?

and thus could avoid a breaking change if wanted

I think changing the enum in any way might count as a breaking change. In any case, as mentioned earlier, we need to make a breaking change to add the Date type anyway, so we're not constrained by that.

Do you see us keeping both Validated and unvalidated enum types, or do you intend to remove the unvalidated ones?

Initially it was to keep the amount of touched LOC low, then I thought it might be nicer to transition over. Forgot at that moment that you either way wanted to introduce a breaking change.

One could argue that the "struct wrappers" are not needed for all the enum variants, like Integer(i64) can already only accept a valid 64-bit integer.
But I would say with Rust's .into() and the proper From traits implemented it's an okay-ish overhead to have all of them in their value type wrappers, so that we keep them consistent. Only outlier here is the Decimal as it's basically already a value type that we receive.

Idea: let's rename the current enum members to something like Raw* / Unvalidated* / Unsafe* members. The migration path is really easy in an existing codebase via search&replace. Then we can have the new and recommended way with the same names as the current enum members, use the breaking change for that, yet upgrade migration isn't a pain and users can gradually transition to validated bare items. WDYT?

let's rename the current enum members to something like Raw* / Unvalidated* / Unsafe* members.

Would they still be in the same enum as the validated types? Either way, I'd love to look at some code.

The IETF list also has some suggestions about adding #[non_exhaustive] to BareItem
If you don't add it now I'll do it later when adding the Date type.

https://lists.w3.org/Archives/Public/ietf-http-wg/2023JanMar/0034.html

Just as a quick update from some work on the weekend:

I decided it's not really worth it to keep both the "unsafe" variants and the validated ones at the same time, as it becomes a bit weird to deal with. The parsing would have yielded "unsafe" variants, even though the parser validated that they're correct.

So the code change will be a bit bigger, as it changes most of the test invocations from .into() to .try_into(), etc. Also some of the tests will need slight changes where errors were expected during the serialization. Now they fail when the value struct gets created.

I'm right now fighting with the String value struct, as I can't easily use the parse_string function when using TryInto, as you may or may now have it wrapped with quotes and the parser expects "double escaped" values in string (like literal values \" which are "\\\"" as a rust string) whereas it's totally fine to have a quote " ("\"") as a string input which can be serialized.

Edit: The problem here was to expect that we pass parseable inputs. This was a wrong assumption on my end. We need serializable values and therefore use the Serializer::serialize_* methods now internally to check for that. It's a bit wasteful, but can be optimized later.

Once that's done I'll open up this PR for review, with one commit per BareItem enum variant being moved to the value structs.

P.S.: Didn't know about the #[non_exhaustive], that's cool!

@valenting I updated the PR description to explain the work that has happened. Let me know if there's anything controversial to it. Specifically I'm interested what you think about the whole .try_into() and .into() dance, which is now necessary to construct field values manually.

I'm happy to be pointed to whatever you think is worth changing, I'm sure there's something I missed 🙂

Allowed ways to construct a BareItem per variant

// BareItem::Decimal
let value = BareItem::new_decimal_from_f64(13.37)?;
let value: BareItem = 13.37.try_into()?;

let decimal = rust_decimal::Decimal::from_f64(13.37).unwrap();
let value = BareItem::new_decimal(decimal);

let value: BareItem = decimal.try_into()?;

// BareItem::Integer
let value = BareItem::new_integer(42)?;
let value: BareItem = 42.try_into()?;

// BareItem::String
let value = BareItem::new_string("foo")?;
// No `.try_into()`, as it's ambiguous whether `Token` or `String` variant should be used

// BareItem::ByteSeq
let value = BareItem::new_byte_seq("hello".as_bytes())?;
let value: BareItem = "hello".as_bytes().try_into()?;

// BareItem::Boolean
let value = BareItem::new_boolean(true)?;
let value: BareItem = true.try_into()?;

// BareItem::Token
let value = BareItem::new_token("foo")?;
// No `.try_into()`, as it's ambiguous whether `Token` or `String` variant should be used

The recommended way would be to use the BareItem::new_* constructors, but I do believe the TryInto variants are quite useful still when creating an Item right away, like:

Item::new(true.try_into()?);