Skip to content

Fix clippy error - #3133

Merged
0xPoe merged 3 commits into
rust-lang:masterfrom
chansuke:hotfix/fix-clippy
Jan 28, 2023
Merged

Fix clippy error#3133
0xPoe merged 3 commits into
rust-lang:masterfrom
chansuke:hotfix/fix-clippy

Conversation

@chansuke

Copy link
Copy Markdown
Contributor

Fix clippy error.

@0xPoe0xPoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot!

If you can separate your changes with the lint name by commit, it would be great.

Comment threadsrc/cli/self_update.rs
@chansuke

Copy link
Copy Markdown
ContributorAuthor

@hi-rustin I have updated the commit log with clippy name. Some of the messages are duplicated because cargo clippy doesn't show the error at once and I procedurally fixed the log:pray:

@chansuke
chansuke requested a review from 0xPoeJanuary 22, 2023 07:00

@0xPoe0xPoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please squash your first and third commits? I think they are from the same lint.
rest LGTM. Thanks again!

Comment threaddownload/src/lib.rs Outdated
@@ -1,5 +1,6 @@
//! Easy file downloading
#![deny(rust_2018_idioms)]
#![allow(clippy::type_complexity)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer to warn it. We may find a better way to fix it in the future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@chansuke Could you please remove this? Then we can merge this PR.

@0xPoe

Copy link
Copy Markdown
Member

Also, could you please rebase your base branch? It seems you still using the 2022 master branch.

@chansuke

Copy link
Copy Markdown
ContributorAuthor

Sorry for many time. I will fix this tonight(JST)

@0xPoe0xPoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks again. I'm sorry for this back-and-forth discussion. I should have said it all at once.
Because this is a nit change. I'll push it directly.

Comment threaddownload/src/lib.rs Outdated
}
}

type EventResult<'a> = &'a dyn Fn(Event<'_>) -> Result<()>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We had some discussions on this change before: #3096 (comment)

Suggested change
typeEventResult<'a> = &'adynFn(Event<'_>) -> Result<()>;
typeDownloadCallback<'a> = &'adynFn(Event<'_>) -> Result<()>;

Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@chansuke

Copy link
Copy Markdown
ContributorAuthor

No problem. Thank you so much for your help.

Sign up for freeto 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

@chansuke@0xPoe