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

Hassle-free rustfmt setup #155

Closed
matklad opened this issue Oct 23, 2018 · 8 comments · Fixed by #271
Closed

Hassle-free rustfmt setup #155

matklad opened this issue Oct 23, 2018 · 8 comments · Fixed by #271
Labels
E-medium fun A technically challenging issue with high impact

Comments

@matklad
Copy link
Member

matklad commented Oct 23, 2018

It would be cool to have all the code formatted with rustfmt. The problem is that setting up rustfmt is not-trivial (mainly because you need to know the precise version). I think a (probably slightly over-engineered) solution to this is to add format command to tools which should

  • install rustfmt for a specific version of rust (command can assume rustup is used)
  • run rustfmt on all code

The end goal is that git clone ... && cd rust-analyzer && cargo format always works.

As a bonus, add cargo format --instal-hook, which installs pre-commit hook for reformatting changed files.

As an extra bonus, package that as a reusable crate.

@matklad matklad added help wanted E-medium fun A technically challenging issue with high impact labels Oct 23, 2018
@kjeremy
Copy link
Contributor

kjeremy commented Oct 23, 2018

We should probably have stuff in our rustfmt.toml as well

@mominul
Copy link
Contributor

mominul commented Oct 24, 2018

As I have worked on the tools crate earlier, I'd like to take this issue!

@alanhdu
Copy link
Contributor

alanhdu commented Oct 26, 2018

For installing the right version of rustfmt, if we're assuming that rustup is installed can we just used rust-toolchain to specify the right version or Rust and Rustfmt to use? Or is the tool supposed to do something like:

  1. Push nightly-2018-10-xx onto override stack
  2. rustup component add rustfmt-preview (if necessary)
  3. Run cargo fmt
  4. Pop nightly-2018-10-xx off of the override stack to use whatever version dev had before

?

@CAD97
Copy link
Contributor

CAD97 commented Oct 26, 2018

rustup install nightly-2018-xx-xx
rustup component add rustfmt-preview --toolchain nightly-2018-xx-xx
cargo +nightly-2018-xx-xx fmt

Should be sufficient. Both rustup install and rustup component add return a success code without doing work when the said toolchain/component is already installed.

@matklad
Copy link
Member Author

matklad commented Oct 26, 2018

@alanhdu more like the second approach, though, instead of overrides, you can directly call rustup run toolchain-version -- cargo fmt.

I don't want to pick a particular blessed rust version, I think it is OK if you, for example, just use the current nightly. For formatting though, it is important to pin the version.

@matklad
Copy link
Member Author

matklad commented Nov 5, 2018

Status update: we now have cargo format which reformats everything. The only thing we need is a one-time cargo format --install-commit-hook which adds cargo format as a pre-commit hook.

Unfortunatelly, we cant' just commit hooks to the repository.

@mominul
Copy link
Contributor

mominul commented Nov 5, 2018

Unfortunatelly, we cant' just commit hooks to the repository.

I know we haven't implemented it yet, are there any restrictions you talking about?

@DJMcNab
Copy link
Contributor

DJMcNab commented Nov 5, 2018

I think he means that we can't create a commit containing a .git/hooks/pre-commit file, as git doesn't support that.

The precommit hook must be manually installed

bors bot added a commit that referenced this issue Dec 9, 2018
271: Implement format hook r=matklad a=DJMcNab

Tentatively: fixes #155.

However, this does add all changes in staged files, which might not be desirable. However, I think we can't solve that without explicit support in rustfmt for it, so it should be fine.

Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
@bors bors bot closed this as completed in #271 Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium fun A technically challenging issue with high impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants