Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clippy:derivable_impls and other lints #10

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

Enselic
Copy link
Contributor

@Enselic Enselic commented Feb 28, 2022

Using https://github.com/Enselic/cargo-public-items we can easily see the impact on the public API of this change, and it is as follows:

% git checkout master
% cargo public-items > /tmp/master
% git checkout fix-some-lints
% cargo public-items > /tmp/pr
% diff -U0 /tmp/master /tmp/pr
--- /tmp/before	2022-02-28 21:10:55.000000000 +0100
+++ /tmp/after	2022-02-28 21:10:44.000000000 +0100
@@ -15 +15 @@
-pub fn bugreport::collector::CommandLine::default() -> Self
+pub fn bugreport::collector::CommandLine::default() -> CommandLine
@@ -21 +21 @@
-pub fn bugreport::collector::CompileTimeInformation::default() -> Self
+pub fn bugreport::collector::CompileTimeInformation::default() -> CompileTimeInformation
@@ -30 +30 @@
-pub fn bugreport::collector::OperatingSystem::default() -> Self
+pub fn bugreport::collector::OperatingSystem::default() -> OperatingSystem
@@ -34 +34 @@
-pub fn bugreport::collector::SoftwareVersion::default() -> Self
+pub fn bugreport::collector::SoftwareVersion::default() -> SoftwareVersion

and as we can see the only difference is that the return type has changed from Self to explicit types, which is equivalent to the current public API.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

There is one (hypothetical?) drawback of #[derive(Default)]: If a future developer adds new fields to one of these structs, they are not necessarily forced to think about whether or not the "canonical" default value is really what they want as the default value for this particular application. Having a custom impl Default forces developers to think about it because it requires a change.

But this is pretty weak argument compared to the benefit of not having to write this boilerplate code. And not the actual reason why I wrote them in the first place (I guess I just wasn't used to #[derive(Default)] yet) 😄

@sharkdp sharkdp merged commit b1627d9 into sharkdp:master Mar 1, 2022
@Enselic Enselic deleted the fix-some-lints branch March 1, 2022 07:57
@Enselic
Copy link
Contributor Author

Enselic commented Mar 1, 2022

Good point about devs not being forced to think about defaults if they add fields. I didn't think of that. But I agree that getting rid of boilerplate trumps that :)

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.

2 participants