Make ScheduleAt special + Typespace::is_valid_for_client_code_generation
#1590
Conversation
| // This better be an option type | ||
| if let Some(inner_ty) = sum_type.as_option() { | ||
| write!(f, "{}?", ty_fmt(ctx, inner_ty, namespace)) | ||
| if sum_type.is_special() { |
There was a problem hiding this comment.
Looking at those if is_... { if let Some(...) ... I get an impression that we would be better served by as_special() + an enum instead of a simple is_special() -> bool.
Then you could use it with a much cleaner code like
match ty.as_special() {
Some(SpecialType::Identity) => ...,
Some(SpecialType::Option(ty)) => ...,
...
None => ...handle non-special types...
}
There was a problem hiding this comment.
I'd even go further and define a type AlgebraicTypeForGenerate which specifies these special variants as enum variants, and where scalars are separated out to their own type. But, this seems like a refactoring we can do later.
There was a problem hiding this comment.
That's really the right way to do all this yeah. We could add AlgebraicTypeUseForGenerate and AlgebraicTypeDefForGenerate, even. And TypespaceForGenerate.
| @@ -874,7 +855,11 @@ impl TableType for TestD { | |||
|
|
|||
|
|
|||
| impl TestD { | |||
| } | |||
| #[allow(unused)] | |||
| pub fn filter_by_test_c(test_c: Option::<Namespace.testC>) -> TableIter<Self> { | |||
There was a problem hiding this comment.
FWIW we decided not to allow filtering by optionals for now back in the consistent filtering proposal (https://github.com/clockworklabs/SpacetimeDBPrivate/pull/783).
So this method shouldn't have appeared.
| @@ -693,6 +690,10 @@ impl RepeatingTestArg { | |||
| #[allow(unused)] | |||
| pub fn find_by_scheduled_id(scheduled_id: u64) -> Option<Self> { | |||
| Self::find(|row| row.scheduled_id == scheduled_id) | |||
| } | |||
| #[allow(unused)] | |||
| pub fn filter_by_scheduled_at(scheduled_at: ScheduleAt) -> TableIter<Self> { | |||
There was a problem hiding this comment.
This got added in Rust output, but not other langs, creating inconsistency between language APIs.
Let's exclude it for now - if we want to allow more types in addition to what we decided in the consistent filtering proposal, we should do this (ugh can't avoid repeating that word) consistently across all SDKs.
There was a problem hiding this comment.
Yeah, I was just updating everywhere is_special was used but it looks like the logic is structured a lil differently across generators. Will revert.
| // This better be an option type | ||
| if let Some(inner_ty) = sum_type.as_option() { | ||
| write!(f, "{}?", ty_fmt(ctx, inner_ty, namespace)) | ||
| if sum_type.is_special() { |
There was a problem hiding this comment.
I'd even go further and define a type AlgebraicTypeForGenerate which specifies these special variants as enum variants, and where scalars are separated out to their own type. But, this seems like a refactoring we can do later.
|
I've fixed this partway but it looks like the typescript SDK needs to be updated with a |
bfops
added
release-any
kazimuth
force-pushed
the
jgilles/more_special
branch
from
cdc02bc to
7d4b194
Compare
|
clockworklabs/spacetimedb-typescript-sdk#88 contains the typescript side of the new changes here. I still need to do an end-to-end test of this with that. |
Update code generation to handle ScheduleAt being special Update typescript generation Address missed review comment
kazimuth
force-pushed
the
jgilles/more_special
branch
from
5a7e199 to
ed21c7b
Compare
Did we ever add it to the C# SDK? When trying to regenerate BitCraft files, I'm getting compilation errors due to |
Description of Changes
This PR does 2 things:
It makes the
ScheduleAttype special, and reworks theis_specialinfrastructure a little. Now we haveSumType::is_specialandAlgebraicType::is_specialin addition toProductType::is_special. I also reorderedScheduleAtso that its variants are in alphabetical order, in preparation for the upcoming "default element ordering" changes.It renames some things in sats to get rid of the name "nominal normal form", which by all accounts
was confusing to everybody. The new APIs are named:
AlgebraicType::is_valid_for_client_type_definitionAlgebraicType::is_valid_for_client_type_useTypespace::is_valid_for_client_code_generationAPI and ABI breaking changes
This is an API and ABI break.
Expected complexity level and risk
The new
AlgebraicType::is_specialmay also need to be integrated in various places.Testing
Running test suite, I'd also like to check how badly this breaks Bitcraft & the SDKs...