feather-rs / feather

A Minecraft server implementation in Rust
Apache License 2.0
2.58k stars 143 forks source link

Ignore derivable_impls lint in Title #471

Closed Iaiao closed 2 years ago

Iaiao commented 2 years ago

Ignore derivable_impls lint in Title

Status

Description

derivable_impls is a new clippy lint, and in this case it's better to ignore it because if we want to add something to Title, we'll forget about derive(Default) and that may lead to unwanted behavior.

Related issues

Checklist

Note: if you locally don't get any errors, but GitHub Actions fails (especially at clippy) you might want to check your rust toolchain version. You can then feel free to fix these warnings/errors in your PR.

Iaiao commented 2 years ago

Looks like GitHub Actions hasn't updated clippy yet

ambeeeeee commented 2 years ago

I'm not sure if either should have a Default implementation in the first place. But either way I don't know if this is the way we should go.

Title should probably just have a builder, if anything. The default implementation provides no value as it would result in a title that never actually gets displayed in its current form. So it's not even usable for the Title { title: Some(String::from("hello")), .. } syntax because you'd just get garbage that never displays.

It's also arguable that we shouldn't have a default implementation for HotbarSlot at all (just like above) because the type is meant to indicate the hotbar slot the entity has selected, and it should be manually created rather than assuming a default value. This is further evidenced by the fact we never actually use HotbarSlot::default() in the code.
If HotbarSlot has a default impl, it should just be #[derive(Default)]. The reason provided isn't really enough to justify not using that derive, because it's likely we will never actually change HotbarSlot.