transpile: Run rustfmt on generated .rs files
#1458
Conversation
Rua
force-pushed
the
rustfmt
branch
3 times, most recently
from
cc8a44b to
f519f02
Compare
| if !status.map_or(false, |status| status.success()) { | ||
| warn!("rustfmt failed, code may not be well-formatted"); | ||
| } |
There was a problem hiding this comment.
Can we handle these errors differently? The command will fail if rustc isn't found or something, and the status will fail if rustc exits with an error. That said, they shouldn't really fail, so I think we can just panic here, maybe with an .expect.
Also, if we're calling rustfmt here, we can remove the rustfmt run in snapshots.rs.
There was a problem hiding this comment.
I don't think a panic is appropriate, as this is just an extra convenience for the user and not a hard requirement of the transpilation process. The transpilation should still succeed if rustfmt is not installed or doesn't complete for whatever reason. Though unlikely, rustfmt may not be installed even if cargo and rustc are.
Rua
force-pushed
the
rustfmt
branch
2 times, most recently
from
3c732f2 to
9e83e14
Compare
| let output_path = build_dir.join(file_name); | ||
| let output = reg.render("lib.rs", &json).unwrap(); | ||
| let path = maybe_write_to_file(&output_path, output, tcfg.overwrite_existing)?; | ||
| rustfmt(&output_path, build_dir); |
There was a problem hiding this comment.
Could we make this optional (with a command line argument)?
There was a problem hiding this comment.
Why would we not do it, though?
There was a problem hiding this comment.
The user might want a custom formatting configuration. They could add that to rustfmt.toml, but that creates a race between adding that file and running the transpiler.
Another reason is performance: what if the user wants to run the transpiler, a bunch of refactoring steps, then run the formatter?
And what if the user just doesn't want to run it at all?
There was a problem hiding this comment.
The user might want a custom formatting configuration. They could add that to
rustfmt.toml, but that creates a race between adding that file and running the transpiler.
They can still run that after the transpiler (which would run rustfmt).
Another reason is performance: what if the user wants to run the transpiler, a bunch of refactoring steps, then run the formatter?
There are plenty of places in the transpiler we don't care about performance, and rustfmt is very fast. It's never going to be the bottleneck compared to the rest of the transpiler.
And what if the user just doesn't want to run it at all?
We already don't make any guarantees about the formatting of the output.
There was a problem hiding this comment.
I realized that rustfmt could occasionally have effects, like eating a comment, and it might interact with how comment insertion will be done, so leaving it as an on-by-default option is better actually.
3 participants
rustfmtwhen translating #742.Rather than running
cargo fmt, this runs therustfmtbinary directly so that it can be used on individual files, and also when there are no build files.