Derive default for api request structs, move type enums #2245
Conversation
crates/api_common/src/comment.rs
Outdated
| @@ -69,7 +69,7 @@ pub struct CreateCommentLike { | |||
| pub auth: Sensitive<String>, | |||
| } | |||
|
|
|||
| #[derive(Debug, Serialize, Deserialize)] | |||
| #[derive(Debug, Serialize, Deserialize, Default)] | |||
There was a problem hiding this comment.
Deriving default is useful for client development, when you just want to specify one or two params, and not put all the None values explicitly.
There are still some structs where derive(Default) would make sense, such as CreateSite. In cases like that, there is a mandatory string value which would automatically be filled with "" if default is used. Still, it would be nice if there was a way to avoid writing out all 14 lines to init that struct. Maybe default would be okay if we ensure that empty strings are rejected.
There was a problem hiding this comment.
I'd probably prefer all of them to derive default, and just make sure the string cases are done by convention. API clients should be will likely find those issues during development anyway.
| @@ -4,7 +4,7 @@ use std::{ | |||
| ops::{Deref, DerefMut}, | |||
| }; | |||
|
|
|||
| #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)] | |||
| #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize, Default)] | |||
There was a problem hiding this comment.
Necessary to make derive(Default) work for some of the structs, even though it obviously doesnt make so much sense. Hopefully the error messages are clear enough. It could also make sense to move the auth param out of the structs somehow.
| Communities, | ||
| Users, | ||
| Url, | ||
| } |
There was a problem hiding this comment.
These enums are very useful for api clients, and should be available without the full feature, so i had to move them.
Nutomic
force-pushed
the
api-derive-default
branch
from
f95b1e2
to
ed59f5f
Compare
|
Added another commit to remove stringly typing from the api, and use enums directly. |
Nutomic
force-pushed
the
api-derive-default
branch
2 times, most recently
from
9056e40
to
15a05e5
Compare
| pub type_: Option<String>, | ||
| pub sort: Option<String>, | ||
| pub type_: Option<ListingType>, | ||
| pub sort: Option<SortType>, |
There was a problem hiding this comment.
This is an API change, and would require the front end clients to use these enums. I'm not against it, just know that its a breaking change
There was a problem hiding this comment.
Also, just to be sure, remove the from_opt_str_to_opt_enum function, that way we can get rid of all of these.
There was a problem hiding this comment.
No, clients work just as before, because serde parses the strings into the correct enum variant. I already tested locally with lemmy-ui, and different sort/listing options worked without any problems.
There was a problem hiding this comment.
from_opt_str_to_opt_enum function is already removed.
There was a problem hiding this comment.
That's kind of wild, because every language does enums differently, and a lot don't even have the concept of string enums. In typescript you have to manually define them as string enums, not the standard integer ones. Okay then, hopefully it doesn't break clients, but we'll know when we cut the RC's.
Also I would still want to change lemmy-js-client to use the enums rather than strings as they do now. I'll open up an issue for that.
crates/api_common/src/comment.rs
Outdated
| @@ -69,7 +69,7 @@ pub struct CreateCommentLike { | |||
| pub auth: Sensitive<String>, | |||
| } | |||
|
|
|||
| #[derive(Debug, Serialize, Deserialize)] | |||
| #[derive(Debug, Serialize, Deserialize, Default)] | |||
There was a problem hiding this comment.
I'd probably prefer all of them to derive default, and just make sure the string cases are done by convention. API clients should be will likely find those issues during development anyway.
Nutomic
force-pushed
the
api-derive-default
branch
2 times, most recently
from
cbf7a58
to
b4a3ac5
Compare
|
Okay, added clone to all api structs, and default to all request (not response) structs (except for a few which only have a single field). |
Nutomic
force-pushed
the
api-derive-default
branch
from
b4a3ac5
to
27b3d2c
Compare
No description provided.