Fix some default column value bugs - #5630
Conversation
…olidate-default-tests' into bfops/fix-defaults
| use spacetimedb::table::TableInternal; | ||
| use spacetimedb::AlgebraicValue; | ||
|
|
||
| #[spacetimedb::table(accessor = defaults)] |
There was a problem hiding this comment.
I think we should either
- remove this file
- extend it to cover all default datatypes
I don't have a strong opinion about which, but I would like the reviewer to have one.
There was a problem hiding this comment.
Doesn't the custom_defaults smoketest already test this behavior? With that understanding, I'd like to remove this file and keep the tests in the smoketest.
There was a problem hiding this comment.
it does in a sense, just a bit less directly; it doesn't check that it gets coerced to the right
AlgebraicValue, but if you think it's close enough to be redundant (or maybe just doesn't matter) then I'm happy to remove this.
There was a problem hiding this comment.
The smoketest does check that the default value in a newly-inserted row is the same as the declared one, right? I don't see how that test could pass while this one fails.
| const EXPECTED_DEFAULTS: &[(&str, &str)] = &[ | ||
| ("bool_value", "true"), | ||
| ("u8_value", "8"), | ||
| // TODO: uncomment this once negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5622 | ||
| //("i8_value", "-8"), | ||
| ("u16_value", "16"), | ||
| // TODO: uncomment this once negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5622 | ||
| //("i16_value", "-16"), | ||
| ("u32_value", "32"), | ||
| // TODO: uncomment this once negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5622 | ||
| //("i32_value", "-32"), | ||
|
|
||
| // TODO: uncomment once u64s are fixed. | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5623 | ||
| //("u64_value", "64"), | ||
|
|
||
| ("u64_value", "64"), | ||
| // TODO: uncomment this once negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5622 | ||
| //("i64_value", "-64"), | ||
|
|
||
| // TODO: uncomment once floats are fixed in C# and f32s are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5624 | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5627 | ||
| //("f32_positive_value", "32.5"), | ||
|
|
||
| // TODO: uncomment once floats are fixed in C#, f32s are fixed in Rust, and negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5624 | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5627 | ||
| ("f32_positive_value", "32.5"), | ||
| // TODO: uncomment once negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5622 | ||
| //("f32_negative_value", "-32.5"), | ||
| ("f64_positive_value", "64.25"), | ||
| // TODO: uncomment once negative defaults are fixed in Rust | ||
| // https://github.com/clockworklabs/SpacetimeDB/issues/5622 | ||
| //("f64_negative_value", "-64.25"), | ||
|
|
||
| // TODO: uncomment this once string default values are fixed in Rust | ||
| //("string_value", r#""default string""#), | ||
| ]; |
There was a problem hiding this comment.
I uncommented the negative value tests and ran locally and they passed. I believe the problem with negative values was:
#val.serialize(...)expanded to e.g.-32.serialize(...).- After macroexpansion, that parsed as
- (32.serialize(...)).
You change makes it so that the macro now correctly constructs the negation operator to the integer first, and then converts it into AlgebraicValue, instead of trying to apply the negation operator afterwards.
There was a problem hiding this comment.
oh fantastic! thank you
| use spacetimedb::table::TableInternal; | ||
| use spacetimedb::AlgebraicValue; | ||
|
|
||
| #[spacetimedb::table(accessor = defaults)] |
There was a problem hiding this comment.
Doesn't the custom_defaults smoketest already test this behavior? With that understanding, I'd like to remove this file and keep the tests in the smoketest.
2 participants
Description of Changes
Fixes #5623
Fixes #5624
Fixes #5627
Fixes #5622
API and ABI breaking changes
No. Fixes two build failures.
Expected complexity level and risk
2
Testing