Conversation
Created using spr 1.3.6-beta.1
|
(marking as draft since it's not hugely important and we might go in a completely different direction) |
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
did a pass; thanks for looking at this
| // Reject contradictory bounds (min > max). | ||
| if let (Some(lo), Some(hi)) = (min, max) { | ||
| if lo > hi { | ||
| return Err(Error::InvalidSchema { | ||
| type_name: type_name.into_option(), | ||
| reason: format!("minimum ({}) is greater than maximum ({})", lo, hi,), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if we want to model this as an error or Never; consider this example:
{
"allOf": [
{
"type": "object",
"properties": {
"number": {
"type": "integer",
"minimum": 10
}
}
},
{
"type": "object",
"properties": {
"number": {
"type": "integer",
"maximum": 5
}
}
}
]
}... and imagine that the two parts of the allOf are useful schemas (referenced) in their own right and the objects contain other fields.
These two properties would merge to a value that was unsatisfiable: there is no value for the number property that is valid, but the overall object is still representable e.g. like:
pub struct Foo {
number: Option<Never>,
}| let max_is_exact = max.map_or(true, |m| (m - *imax).abs() <= f64::EPSILON); | ||
|
|
||
| if min == Some(1.) | ||
| && (max_is_exact || get_type_name(&type_name, metadata).is_none()) |
There was a problem hiding this comment.
we should usually have a type name we can apply -- we should have a test for this case
| // Bounds match the format type exactly. | ||
| return Ok((TypeEntry::new_integer(ty), metadata)); | ||
| } else if get_type_name(&type_name, metadata).is_some() { | ||
| // Sub-range bounds on a named type: generate a |
| // redundant checks which result in a warning (e.g., | ||
| // `value < 0` on u8). | ||
| // | ||
| // For min >= 1 we could use a NonZero type, but that |
There was a problem hiding this comment.
I'd also say that adds complexity without any particular value.
We could offer conversions to NonZero types... but meh. That's not actually that useful I'd expect.
| if lo > hi { | ||
| return Err(Error::InvalidSchema { | ||
| type_name: type_name.into_option(), | ||
| reason: format!( | ||
| "no valid integers in range \ | ||
| (effective minimum {} > \ | ||
| effective maximum {} after \ | ||
| rounding fractional bounds)", | ||
| lo, hi, | ||
| ), | ||
| }); |
There was a problem hiding this comment.
is this different than the check above?
| // If this is just a wrapper around a string or integer, we can derive | ||
| // some more useful traits. | ||
| if is_str || is_int { | ||
| derive_set.extend(["PartialOrd", "Ord", "PartialEq", "Eq", "Hash"]); | ||
| } | ||
| if is_int { | ||
| derive_set.insert("Copy"); | ||
| } |
There was a problem hiding this comment.
I'd suggest
| // If this is just a wrapper around a string or integer, we can derive | |
| // some more useful traits. | |
| if is_str || is_int { | |
| derive_set.extend(["PartialOrd", "Ord", "PartialEq", "Eq", "Hash"]); | |
| } | |
| if is_int { | |
| derive_set.insert("Copy"); | |
| } | |
| // If this is just a wrapper around a string, we can derive | |
| // some more useful traits. | |
| if is_str { | |
| derive_set.extend(["PartialOrd", "Ord", "PartialEq", "Eq", "Hash"]); | |
| } | |
| if is_int { | |
| derive_set.extend(["PartialOrd", "Ord", "PartialEq", "Eq", "Hash", "Copy"]); | |
| } |
| Named integer types with sub-range bounds (e.g., a `uint8` with `maximum: 63`) | ||
| generate constrained newtypes with `TryFrom` validation. Anonymous integer | ||
| properties with bounds that don't match a standard Rust type are not yet | ||
| constrained. |
There was a problem hiding this comment.
Truly anonymous types are rare (and arguably bugs). Note that this is under the WIP heading so we may want to rephrase with that in mind.
2 participants
Track and handle bounded integers using newtypes. (Not floats yet, though.)
This is a pretty large PR, though most of it is tests -- I hope the general intent is clear.