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

Some small improvements #2059

Closed
wants to merge 6 commits into from

Conversation

KlossPeter
Copy link

Mostly clippy suggestions, some might improve perf (as removing unneeded clones), most should improve readability.

Copy link
Contributor

@tesuji tesuji left a comment

Choose a reason for hiding this comment

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

You need to run cargo fmt on finish.

src/dist/download.rs Outdated Show resolved Hide resolved
src/dist/config.rs Outdated Show resolved Hide resolved
src/dist/download.rs Outdated Show resolved Hide resolved
@kinnison
Copy link
Contributor

These look pretty good, but I don't like a commit labelled rustfmt in the middle. Could you please rebase, folding the formatting into the commits which need them? Once that's done, I think this can be merged. 👍

@bors
Copy link
Contributor

bors commented Oct 26, 2019

☔ The latest upstream changes (presumably #2086) made this pull request unmergeable. Please resolve the merge conflicts.

@kinnison
Copy link
Contributor

kinnison commented Nov 9, 2019

Hi @KlossPeter

Your branch needs rebasing onto current master as there are conflicts which need resolving. In addition, could you please remove the formatting patch and instead ensure on each commit that cargo fmt has already been run?

You may find it easier to complete the rebase if you make many smaller commits.

Thanks,

Daniel.

@kinnison
Copy link
Contributor

kinnison commented Dec 3, 2019

Hi @KlossPeter

Have you run out of time to work on this, or decided to go and do something else? It's no problem if you have, but if so, could you please close this PR? If instead you want to fix the conflicts and clean up the commit history then I'd be pleased to look again at it.

@kinnison
Copy link
Contributor

I'm having to assume this has been abandoned.

@kinnison kinnison closed this Dec 16, 2019
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.

4 participants