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

Minor codestyle fixes #52

Merged
merged 2 commits into from
Oct 18, 2017
Merged

Conversation

TalhaKhatri
Copy link
Contributor

These are just some minor adjustments that don't change the logic of the library.
Mostly guided by clippy.

@TalhaKhatri TalhaKhatri reopened this Oct 17, 2017
@oconnor663
Copy link
Owner

oconnor663 commented Oct 17, 2017

Ha, I looked at those lints a couple weeks ago, but I think it was just too late in the evening and I didn't have the energy for dealing with it :) I'll go ahead and merge this when CI passes. Thanks for fixing!

What do you think we should do as far as integrating Clippy into CI? There's probably some easy way to get Travis to run it?

@@ -1804,8 +1804,7 @@ impl WaitMode {
// guaranteed to finish soon). Blocking waits should always join, even
// in the presence of errors.
match (self, expression_result) {
(&WaitMode::Blocking, _) => true,
(_, &Ok(Some(_))) => true,
(&WaitMode::Blocking, _) | (_, &Ok(Some(_))) => true,
_ => false,
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Musing unrelated to this fix:

This while should_join_background_thread concept feels really complicated now that I'm getting back to it :( I've been planning to get rid of the Then concept entirely, because of the complications like this that it creates, and it might be that then I can delete this whole thing...

@oconnor663
Copy link
Owner

The OSX builds seem blocked for some reason. Hopefully they'll kick off overnight.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 612a1c7 on TalhaKhatri:codestyle into 3c8c57b on oconnor663:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 612a1c7 on TalhaKhatri:codestyle into 3c8c57b on oconnor663:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 612a1c7 on TalhaKhatri:codestyle into 3c8c57b on oconnor663:master.

@TalhaKhatri
Copy link
Contributor Author

Looks like all the checks have passed. This was actually my first opensource PR! 🎉 I'll have to look into the integration of Clippy in to CI. If I find something, I'll let you know.

@oconnor663 oconnor663 merged commit 544ddfb into oconnor663:master Oct 18, 2017
@oconnor663
Copy link
Owner

Congrats! And thanks for figuring it all out. I just filed #53 for thinking about the CI side of things -- feel free to throw in thoughts there.

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.

None yet

3 participants