Skip to content

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

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

Closed
phansch opened this issue Mar 2, 2019 · 6 comments
Closed
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages

Comments

@phansch
Copy link
Member

phansch commented Mar 2, 2019

As pointed out by @flip1995 in #3824 (comment), we currently don't have an easy way to run rustfmt on the tests locally like we do on CI.

What we are doing on CI currently:

rustup component add rustfmt || cargo install --git https://github.com./rust-lang/rustfmt/ --force

cargo +nightly fmt --all -- --check

# make sure tests are formatted
# some lints are sensitive to formatting, exclude some files
tests_need_reformatting="false"
# switch to nightly
rustup override set nightly
# avoid loop spam and allow cmds with exit status != 0
set +ex
for file in `find tests -not -path "tests/ui/doc.rs" -not -path "tests/ui/unused_unit.rs" | grep "\.rs$"` ; do
rustfmt ${file} --check
if [ $? -ne 0 ]; then
echo "${file} needs reformatting!"
tests_need_reformatting="true"
fi
done
set -ex # reset
if [ "${tests_need_reformatting}" == "true" ] ; then
echo "Tests need reformatting!"
exit 2
fi
# switch back to master
rustup override set master

It would be nice if we could extract that into a separate script so that people can use to format their tests locally, instead of waiting for CI.

Some things to consider when doing this:

  • The script also needs to add the rustfmt component if it isn't present locally
  • We use rustfmt instead of cargo fmt for the test formatting because of cargo fmt doesn't format files in sub-directories in tests rustfmt#1820
  • To make it idempotent, the new script should automatically detect the 'current' toolchain and switch back to it after the formatting script has finished (instead of switching to master)
  • Unlike a fresh VM on CI, We can't just use rustup override set nightly, because that doesn't update the locally installed nightly rust. We should probably pin to a specific nightly instead and update the version from time to time.
  • Using a shell script will not work cross-platform but I think doing it in Rust might be too much work?
@phansch phansch added the C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages label Mar 2, 2019
@phansch
Copy link
Member Author

phansch commented Mar 2, 2019

If we want to provide a cargo format subcommand instead of a shell script, then it might also be worth to look at how rust-analyzer did it: rust-lang/rust-analyzer#163.

@ghost
Copy link

ghost commented Mar 12, 2019

I'm working on adding a clippy dev subcommand to do this.

I'm don't think the command should install/update toolchains/components by default (especially if we want to run it from a test) but I'll provide an option to do that and error out if the required toolchains/components aren't installed and up to date.

@phansch
Copy link
Member Author

phansch commented Jun 18, 2019

friendly ping @mikerite: did you make some progress on this?

@ghost
Copy link

ghost commented Jun 19, 2019

Thanks for the ping.

I didn't get very far and I haven't looked at this in weeks. It's fine by me if someone else wants to do this. Otherwise, I'll try to make some progress this weekend.

@ghost
Copy link

ghost commented Jun 24, 2019

I've got something ready but I'll wait until the build is fixed before submitting the PR.

@ghost ghost mentioned this issue Jun 25, 2019
@phansch
Copy link
Member Author

phansch commented Oct 4, 2019

Closing this - thanks to @mikerite we now have util/dev fmt which has been working great 🎊

@phansch phansch closed this as completed Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages
Projects
None yet
Development

No branches or pull requests

1 participant