Skip to content

/rust-analyzer

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
+47 −28

Conversation

Reviewers

@matklad matklad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mominul @matklad
@mominul
Copy link
Contributor

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!
@mominul
initial implementation
Verified
This commit was signed with a verified signature.
@mominul mominul Muhammad Mominul Huque
GPG key ID: 9D472E8B36098956 Learn about signing commits
Loading status checks…
8ed538a
@matklad

matklad reviewed Oct 30, 2018
View changes

crates/tools/src/main.rs Outdated

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

This comment has been minimized.

Sign in to view
Copy link
@matklad

matklad Oct 30, 2018

Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
@mominul

mominul Oct 31, 2018

Author Contributor

How about we have a const value like:

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

and use it where we need?

This comment has been minimized.

Sign in to view
Copy link
@matklad

matklad Oct 31, 2018

Collaborator

👍
@matklad

matklad reviewed Oct 30, 2018
View changes

crates/tools/src/main.rs Outdated
@@ -163,3 +160,11 @@ fn run(cmdline: &'static str, dir: &str) -> Result<()> {
}
Ok(())
}

fn run_rustfmt() -> Result<()> {

This comment has been minimized.

Sign in to view
Copy link
@matklad

matklad Oct 30, 2018 •
edited

Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

matklad commented Oct 31, 2018

Oh, and one more stratch goal is to have a test which checks formatting.
@mominul
Various changes 
Pin to a specific toolchain version
Format checking functionality
Add a test to check the code formatting.
Remove macro_use attribute
Verified
This commit was signed with a verified signature.
@mominul mominul Muhammad Mominul Huque
GPG key ID: 9D472E8B36098956 Learn about signing commits
Loading status checks…
20175c2
@mominul

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

matklad commented Oct 31, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

mominul commented Oct 31, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

matklad commented Oct 31, 2018

I'd rather merge this as is!

@matklad matklad merged commit 857c165 into rust-analyzer:master Oct 31, 2018
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@matklad

This comment has been minimized.

Sign in to view
Copy link
Collaborator

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

This comment has been minimized.

Sign in to view
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.

@mominul mominul deleted the mominul:format_command branch Nov 1, 2018

@phansch phansch referenced this pull request Mar 2, 2019

Open

Provide an easy way for contributors to run cargo fmt like it's executed on CI #3835

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.