Use newtypes ColId, TableId, IndexId, SequenceId everywhere (*)
#408
Conversation
mamcx
force-pushed
the
move-newtype-ids
branch
from
204edd1 to
2f80be1
Compare
There was a problem hiding this comment.
I know there's some outstanding big chunks of work at least @mamcx needs to get in, I trust you to coordinate about patch ordering.
| unknown => { spacetimedb_sdk::log::error!("Event on an unknown reducer: {:?}", unknown); None } | ||
| } | ||
| match &function_call.reducer[..] { | ||
| "delete_pk_address" => _reducer_callbacks.handle_event_of_type::<delete_pk_address_reducer::DeletePkAddressArgs, ReducerEvent>(event, _state, ReducerEvent::DeletePkAddress), |
There was a problem hiding this comment.
I don't get why this changed? Not a big deal.
There was a problem hiding this comment.
Excellent question I also want the answer to xD
| built_in_into!(f64, F64); | ||
| built_in_into!(&str, String); | ||
| built_in_into!(&[u8], Bytes); | ||
|
|
There was a problem hiding this comment.
Here and some places it looks like ConstraintId isn't handled? I'm assuming ConstraitId is a type that we expect to use a lot more of shortly? And so it's named but not always plumbed all the way through etc
If that take is correct, I'm indifferent between you adding the impl for it here now, or later when it's need.
There was a problem hiding this comment.
Good catch. I have that in my PRs but here this move to another crate so I miss it.
crates/core/src/vm.rs
Outdated
| ST_SEQUENCES_NAME, | ||
| (&StSequenceRow { | ||
| sequence_id: 1, | ||
| sequence_id: SequenceId(1), |
There was a problem hiding this comment.
Mostly my curiosity, I don't demand this sort of consistency: How come sometimes you write 1.into() and sometimes SequenceId(1)?
There was a problem hiding this comment.
This wasn't in my original PR so @mamcx is best placed to answer that after the rebase. :)
There was a problem hiding this comment.
I do it pure by convenience, but should we enforce one way or another?
|
|
||
| [dependencies] | ||
| getrandom = {workspace = true, optional = true} | ||
| spacetimedb-primitives = { path = "../primitives" } |
There was a problem hiding this comment.
Again for my edification: Why does this one not need to name the 0.7.1 tag? Is it because bindings-sys isn't published or something?
There was a problem hiding this comment.
I left it as was before. I also wonder if we require the tag.
Ah I see now that mario pushed to this :) |
mamcx
force-pushed
the
move-newtype-ids
branch
from
2f80be1 to
0d9fe74
Compare
Description of Changes
All cases of e.g.,
col_id/table_id/index_id/sequence_id: u32are done away with and the newtypes are used instead for clearer code and more type safety. Moreover, the change in usingColIdmeans we have to do.clone()and.map(..)less as we can stay inColId-land until we actually have to transform from/intoAlgebraicValue. I hope to eventually do away with all cases of reallocation due to the u32/ColId type difference as the representations are the same so a "transmute" should be possible.The
St*Fieldsenums now also use a macro for their definition to reduce boilerplate and to keep the entire definition in one place (strings, indices, and variant names).API and ABI
If the API is breaking, please state below what will break
The module API breaks because the newtypes is used in the
bindings*crates now.Expected complexity level and risk
I'd rate this PR a 2.5; each change is rather trivial in itself, but there are a lot of them so there's some risk in numbers.
Moreover, there are some nixed reallocations and new allocations.