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

Add explicit return statement for readability #432

Closed
wants to merge 1 commit into from

Conversation

whot
Copy link
Contributor

@whot whot commented Aug 8, 2023

A typo fix and an explicit return to match the surrounding code

Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Nice! thanks so much. CONTRIBUTING didn't convince you to use emoji prefixes? ;)

No functional changes because everything else is behind other #cfg
macros so this is our only effective block here. But those others use
return so it makes the code more consistent.
@whot
Copy link
Contributor Author

whot commented Aug 10, 2023

Nice! thanks so much. CONTRIBUTING didn't convince you to use emoji prefixes? ;)

oops. /me writes "thou shall not submit PRs from airports" 100 times
as an apology, I've now filed #435 with even more typo fixes :)

Also turns out that clippy was unhappy because it's a needless return so I had to add the #[allow(...)] macro to work around that. I'm pretty sure that same clippy error would be triggered on macos for that return there though, so arguably we should just enable that suppression for the whole block.

@whot whot changed the title Two minor cleanups Add explicit return statement for readability Aug 10, 2023
@zeenix
Copy link
Contributor

zeenix commented Aug 10, 2023

Also turns out that clippy was unhappy because it's a needless return so I had to add the #[allow(...)] macro to work around that.

Hmm.. in that case, I'd disagree with Return usage. I prefer to keep #[allow.. use to the minimum.

@whot
Copy link
Contributor Author

whot commented Aug 11, 2023

Yeah, fair enough. Feel free to close this PR though as I said above, I'm pretty sure clippy will fail on macos right now too.

@zeenix
Copy link
Contributor

zeenix commented Aug 11, 2023

Yeah, fair enough. Feel free to close this PR though as I said above, I'm pretty sure clippy will fail on macos right now too.

Just to be clear, you mean with this PR, right? It currently seems to work fine on main. I ran cargo clippy --target x86_64-apple-darwin and I didn't see any warnings.

@whot
Copy link
Contributor Author

whot commented Aug 14, 2023

Hah, indeed, this appears to be a bug in clippy which doesn't warn about single-line returns but it does in a block. i.e.

// this code does not trigger a warning
#[cfg(target_os = "macos")]
return Self::from_str("launchd:env=DBUS_LAUNCHD_SESSION_BUS_SOCKET");
// warning: unneeded `return` statement
#[cfg(target_os = "macos")]
{
    return Self::from_str("launchd:env=DBUS_LAUNCHD_SESSION_BUS_SOCKET"); 
}

Purely from looking at the code I would've expected either to trigger a warning.

(also thanks, now I learned about cargo clippy --target 😄)

@zeenix
Copy link
Contributor

zeenix commented Aug 14, 2023

@whot so we can close this then? Please close it, if you agree.

@whot
Copy link
Contributor Author

whot commented Aug 15, 2023

yep, let's close this

@whot whot closed this Aug 15, 2023
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