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

[WIP] Add format command to tools crate #163

Merged
merged 2 commits into from
Oct 31, 2018

Conversation

mominul
Copy link
Contributor

@mominul mominul commented Oct 26, 2018

I am quite unsure about the implementation of #155 , so I want to get reviews.

Thanks!

Thanks to @alanhdu @CAD97 for discussing the rustup commands!


fn run_rustfmt() -> Result<()> {
// Use beta toolchain for 2018 edition.
run("rustup install beta", ".")?;
Copy link
Member

Choose a reason for hiding this comment

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

Let's pin this to a specific beta, rustup install beta-2018-10-30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we have a const value like:

const TOOLCHAIN: &str = "beta-2018-10-30";

and use it where we need?

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -163,3 +160,11 @@ fn run(cmdline: &'static str, dir: &str) -> Result<()> {
}
Ok(())
}

fn run_rustfmt() -> Result<()> {
Copy link
Member

@matklad matklad Oct 30, 2018

Choose a reason for hiding this comment

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

It would be cool to have an fn run_rustfmt_precommit, which formats all files changed in git. See this for the proper git command invocation and for how to make rustfmt format a single file: https://github.com/antiochp/grin/blob/121cce7e515f9eca8d8708a262a7cc262ee9b7e9/.hooks/pre-commit#L27

@matklad
Copy link
Member

matklad commented Oct 31, 2018

Oh, and one more stratch goal is to have a test which checks formatting.

Pin to a specific toolchain version
Format checking functionality
Add a test to check the code formatting.
Remove macro_use attribute
@mominul
Copy link
Contributor Author

mominul commented Oct 31, 2018

@matklad We now use a specific toolchain version and there is a test to check the code formatting. Can you check it please? I haven't yet added precommit hook though.

@matklad
Copy link
Member

matklad commented Oct 31, 2018

I haven't yet added precommit hook though.

Let's handle that in a separate PR then!

Bascially, I think we need to have

cargo format-hook --install

which installs the hook (copies relevant files to .git/hooks/pre-commit)

and

cargo format-hook

which does the trick.

@matklad
Copy link
Member

matklad commented Oct 31, 2018

I'll merge this PR manually and run a one-time formatting.

@matklad
Copy link
Member

matklad commented Oct 31, 2018

BTW, this seems like a common problem, so publishing this as a reusable rustfmt-manager crate/binary might be useful to other folks!

@mominul
Copy link
Contributor Author

mominul commented Oct 31, 2018

Ok! I'll create a rustfmt-manager crate then. But should we merge this or wait for the crate?

@matklad
Copy link
Member

matklad commented Oct 31, 2018

I'd rather merge this as is!

@matklad matklad merged commit 857c165 into rust-lang:master Oct 31, 2018
@matklad
Copy link
Member

matklad commented Nov 1, 2018

Ok! I'll create a rustfmt-manager crate then. But should we merge this or wait for the crate?

I think it make sense to prototype the git-hook as a PR against this repo, and then extract everyting as a crate: we might find some things to improve after using this for a bit. For example, I've noticed that rustup is not exactly fast, and added some code to check if we already have rustfmt isntalled

@mominul
Copy link
Contributor Author

mominul commented Nov 1, 2018

Ok, I'll start hacking with git pre-commit hook and have a PR here first! After we gain experience, we can bundle them into a crate as you have said.

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