Skip to content

Fix clippy warning #270

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 7, 2019
Merged

Fix clippy warning #270

merged 7 commits into from
Oct 7, 2019

Conversation

k-nasa
Copy link
Member

@k-nasa k-nasa commented Oct 1, 2019

Looking at GitHub actions, it seems that clippy warnnings are accumulating.

Clippy results: 0 ICE, 0 errors, 31 warnings, 0 notes, 0 help

@skade
Copy link
Collaborator

skade commented Oct 1, 2019

Especially the Default implementations are not implemented in stdlib and we should think about following that lint or turning it off instead.

skade
skade reviewed Oct 1, 2019
View reviewed changes
Copy link
Collaborator

@skade skade left a comment

Choose a reason for hiding this comment

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

This looks good! Just flagged something for @stjepang

src/task/block_on.rs
@@ -69,12 +69,11 @@ where
let future = task_local::add_finalizer(future);

let future = async move {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For @stjepang: This is not an equivalent transform. Reading the surrounding code, I also don't think that matters, but can you please revalidate?

Copy link

Choose a reason for hiding this comment

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

This change looks fine by me. It doesn't really matter because res is ().

@k-nasa
Copy link
Member Author

k-nasa commented Oct 1, 2019

@skade I think the default trait should be as much as possible.
The library user should not be saddened by the lack of Default trati.
So I think lint should not be turned off.
How about ❓

@yoshuawuyts yoshuawuyts mentioned this pull request Oct 1, 2019
Merged
@k-nasa k-nasa marked this pull request as ready for review October 3, 2019 09:03
@yoshuawuyts yoshuawuyts changed the title Fix clippy warnning Fix clippy warning Oct 5, 2019
@yoshuawuyts yoshuawuyts requested a review from a user October 5, 2019 20:59
@yoshuawuyts yoshuawuyts mentioned this pull request Oct 5, 2019
Merged
yoshuawuyts
yoshuawuyts approved these changes Oct 6, 2019
View reviewed changes
Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Last open question was resolved; this is looking good!

@yoshuawuyts
Copy link
Contributor

bors r+

@yoshuawuyts yoshuawuyts mentioned this pull request Oct 6, 2019
Merged
@k-nasa
Copy link
Member Author

k-nasa commented Oct 7, 2019

I don't have the right to see it, but it looks like bors is waiting

bors ping

@bors
Copy link
Contributor

bors bot commented Oct 7, 2019

pong

@yoshuawuyts
Copy link
Contributor

Yep, merging manually -- this has been an issue for all PRs for the last day or so.

@yoshuawuyts yoshuawuyts merged commit 5f708f3 into async-rs:master Oct 7, 2019
@k-nasa k-nasa deleted the fix_clippy_warn branch October 8, 2019 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skade skade skade left review comments

@yoshuawuyts yoshuawuyts yoshuawuyts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k-nasa @skade @yoshuawuyts