Skip to content

Conversation

@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jan 30, 2023

Closes #181

There is still one error with progenitor tests (build_nexus), naming that https://doc.rust-lang.org/std/net/struct.Ipv4Addr.html and friends do not implement Default, so any struct which uses them will fail to derive Default. magiclen/educe#6 shows how this can already be fixed with educe, and also how an enhancement to educe might be able to make the implementation here less complicated.

This also highlights there are no tests here using format: ipv4 in such a similar way to progenitor's nexus.json, that should causes failures here.

@jayvdb jayvdb mentioned this pull request Jan 30, 2023
self.uses_uuid = true;
Ok((TypeEntry::new_builtin("uuid::Uuid", &["Display"]), metadata))
Ok((
TypeEntry::new_builtin("uuid::Uuid", &["Default", "Display"]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default Uuid does not seem useful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as noted in the commit 5fabb9b (#184), I am unsure about all of these

TypeEntry::new_builtin("chrono::Date<chrono::offset::Utc>", &["Display"]),
TypeEntry::new_builtin(
"chrono::Date<chrono::offset::Utc>",
&["Default", "Display"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


Some("ip") => Ok((
TypeEntry::new_builtin("std::net::IpAddr", &["Display"]),
TypeEntry::new_builtin("std::net::IpAddr", &["Default", "Display"]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

educe :: Educe,
)]
#[educe(Default)]
pub enum StringEnum {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would this have a default

pub struct ExprString(pub String);
impl std::ops::Deref for ExprString {
type Target = String;
fn deref(&self) -> &String {
&self.0
}
}
#[derive(Clone, Debug, Deserialize, Serialize)]
#[derive(Clone, Debug, Default, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct ExtentTransform {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for example:, but this type clearly doesn't have a default value in the json spec.

#[derive(Clone, Debug, Deserialize, Serialize, educe :: Educe)]
#[educe(Default)]
#[serde(untagged)]
pub enum IdOrName {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would this type have a default? I don't get it.

@ahl
Copy link
Collaborator

ahl commented Jan 30, 2023

The proposal in #181 is a great idea; this implementation seems pretty divergent from that idea.

@jayvdb
Copy link
Contributor Author

jayvdb commented Mar 4, 2023

The proposal in #181 is a great idea; this implementation seems pretty divergent from that idea.

This implementation does solve #181, and attempts to fulfill a basic expectation that data structures in Rust should provide a Default implementation. Obviously this implementation should match the default in the JSON schema if one has been provided, but in all other cases I believe typify should provide a sane Default. There are many other subsets like #181 where a Default is sensible. Rather than solve each one, I tried to add Default to every data structure produced by typify, then it is safe to always add Default to any containers with members using typify generated types.

I am required to add this for my HTTP data structures, because another library demands it. It also seems quite feasible to do, hence this WIP, where only one datatype has no Default to leverage. I am happy to keep going on this approach, but the resistance to it has meant that I am now worried about investing more time into it. Instead, I now load the generated Rust code into syn and post-process the output in order to add Default where it is missing. So I no longer need help from typify to make this possible.

@ahl
Copy link
Collaborator

ahl commented Dec 26, 2024

I think this has been obviated by #725. Please feel free to re-open or file an issue if not.

@ahl ahl closed this Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Structs without any "required" should implement Default

2 participants