Makefile: remove format command in favor of simpler format-check
#4037
Conversation
There was a problem hiding this comment.
This seems fine to me.
I have one concern in current form. If I understand correctly, we can be in a situation where cargo fmt runs but then make format-check fails, because of tab characters? It seems a bit of a regression that there's no command I can run that results in "force everything to conform to the format checker".
Might still need a wrapper bash script around cargo fmt, albeit a simpler one, for the tab issue?
Makefile
Outdated
| @./tools/run_cargo_fmt.sh $(TOCK_FORMAT_MODE) | ||
| @touch tools/.format_fresh | ||
| .PHONY: fmt format-check | ||
| fmt format-check: |
There was a problem hiding this comment.
| fmt format-check: | |
| format-check: |
or, if we're committed to keeping a shorthand fmt-chk, but fmt alone is a verb that formats IMHO, which this no longer does IIUC
There was a problem hiding this comment.
Right, I completely missed this shorthand. Happy to update.
It's a rare enough issue that I don't think we'll hit it much in practice, but I'll let the jury decide. |
I think as long as CI ensures it's fixed we're fine. |
Those commands date to 2016: d4f62cf (Also wow my attitude on commands has changed! Now I say shorter the better! But code should be descriptive.) |
Sure, will re-add. From the discussion in the slack In that case I'll literally alias it to In the future this |
This removes the previous `make format` command which, in essence, runs some bash magic to achieve the exact same job that `cargo fmt` does, if its run within workspace instead of a particular crate. Besides this, many people's editors have a native rustfmt integration anyways, which makes this script and its prompt for unstaged files largely redundant. Instead, we replace it with the more useful `make format-check` and the associated `tools/check_format.sh`. This script does more than `cargo fmt --check` does, for instance also ensuring that no Rust source file contains tab characters (which rustfmt sometimes leaves behind). As part of `make prepush`, `check-format` is now more in line with the other jobs, such as `cargo check` or Clippy: it'll complain, but not touch any source code. To retain compatibility, we introduce a `make format` and `make fmt`, aliased to `cargo fmt` -- without prompting the user. This also removes an optimization that uses Make's logic to check for changed files in order to skip formatting. However, I doubt that this will meaningfully impact users (`make format-check` completes in about 2sec and does not take a meaningful amount of time compared to the other jobs executed as part of `prepush`).
321e836 to
12ae6a1
Compare
|
Should be good to go then! |
Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Pull Request Overview
This removes the
make formatcommand which, in essence, runs some bash magic to achieve the exact same job thatcargo fmtdoes, if its run within workspace instead of a particular crate. Besides this, many people's editors have a native rustfmt integration anyways, which makes this script and its prompt for unstaged files largely redundant.Instead, we replace it with the more useful
make format-checkand the associatedtools/check_format.sh. This script does more thancargo fmt --checkdoes, for instance also ensuring that no Rust source file contains tab characters (which rustfmt sometimes leaves behind).As part of
make prepush,check-formatis now more in line with the other jobs, such ascargo checkor Clippy: it'll complain, but not touch any source code.This also removes an optimization that uses Make's logic to check for changed files in order to skip formatting. However, I doubt that this will meaningfully impact users (
make format-checkcompletes in about 2sec and does not take a meaningful amount of time compared to the other jobs executed as part ofprepush).Testing Strategy
CI / running make commands.
TODO or Help Wanted
N/A
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.