Below are things noticed with defaults after running on cargo-script (NB: this is just a log of things I noticed that I wouldn't accept/leave default for my code, in addition to outright bugs). I've also noted where changing to the following config solved the issue (FWC: "fixed with config"):
max_width = 10000
reorder_imports = true
format_strings = false
# Not *entirely* sure how much these do.
where_layout = "Vertical"
where_pred_indent = "Tabbed"
# These don't appear to *do* anything:
chain_base_indent = "Tabbed"
fn_arg_indent = "Tabbed"
ideal_width = 78-
Block comments are being turned into line comments. Stahp!
-
Handling of block comments is somewhat wonky;
/**!→// !(#553), plus it adds a spurious blank line afterward (#?). Then again,/**…*/comments don't appear to be touched (#?). -
(FWC) Wrapping lines in comments seems to fail constantly; output is a mess of "Rustfmt failed at …; line exceeded maximum length (sorry)" (#503?).
-
Leading/trailing
|inversion in match arms.This is actually old style for me; now, I actually put the
=>on a new line entirely to try and keep additions from clobbering existing lines.
impl MainError {
pub fn is_human(&self) -> bool {
use self::MainError::*;
match *self {
- Io(blame, _)
- | OtherOwned(blame, _)
- | OtherBorrowed(blame, _) => blame == Blame::Human,
+ Io(blame, _) |
+ OtherOwned(blame, _) |
+ OtherBorrowed(blame, _) => blame == Blame::Human,
}
}
}- Forcing attributes to a separate line; I put
#[macro_use]crates in a block with each on a single line:
-#[macro_use] extern crate lazy_static;
-#[macro_use] extern crate log;
+#[macro_use]
+extern crate lazy_static;
+#[macro_use]
+extern crate log;- Everything it did to my
clapbuilder expression is horrible and awful. Went right off the side of the editor window. At a minimum, it doubled the indentation at each level, smearing some simple expressions over multiple lines. Just... eugh.
// We have to kinda lie about who we are for the output to look right...
let m = App::new("cargo")
- .bin_name("cargo")
- .version(version)
- .about(about)
- .arg_required_else_help(true)
- .subcommand_required_else_help(true)
- .subcommand(SubCommand::with_name("script")
- .version(version)
- .about(about)
- .usage("cargo script [FLAGS OPTIONS] [--] <script> <args>...")
- .arg(Arg::with_name("script")
- .help("Script file (with or without extension) to execute.")
- .index(1)
- )
- .arg(Arg::with_name("args")
- .help("Additional arguments passed to the script.")
- .index(2)
- .multiple(true)
- )
- .arg(Arg::with_name("expr")
- .help("Execute <script> as a literal expression and display the result.")
- .long("expr")
- .short("e")
- .conflicts_with_all(csas!["loop"])
- .requires("script")
- )
- .arg(Arg::with_name("loop")
- .help("Execute <script> as a literal closure once for each line from stdin.")
- .long("loop")
- .short("l")
- .conflicts_with_all(csas!["expr"])
- .requires("script")
- )
- .arg_group(ArgGroup::with_name("expr_or_loop")
- .add_all(&["expr", "loop"])
- )
- .arg(Arg::with_name("clear_cache")
- .help("Clears out the script cache.")
- .long("clear-cache")
- )
- .arg(Arg::with_name("count")
- .help("Invoke the loop closure with two arguments: line, and line number.")
- .long("count")
- .requires("loop")
- )
- .arg(Arg::with_name("pkg_path")
- .help("Specify where to place the generated Cargo package.")
- .long("pkg-path")
- .takes_value(true)
- .requires("script")
- .conflicts_with_all(csas!["clear_cache", "force"])
- )
- .arg(Arg::with_name("gen_pkg_only")
- .help("Generate the Cargo package, but don't compile or run it.")
- .long("gen-pkg-only")
- .requires("script")
- .conflicts_with_all(csas!["args", "build_only", "debug", "force"])
- )
- .arg(Arg::with_name("build_only")
- .help("Build the script, but don't run it.")
- .long("build-only")
- .requires("script")
- .conflicts_with_all(csas!["args"])
- )
- .arg(Arg::with_name("debug")
- .help("Build a debug executable, not an optimised one.")
- .long("debug")
- .requires("script")
- )
- .arg(Arg::with_name("dep")
- .help("Add an additional Cargo dependency. Each SPEC can be either just the package name (which will assume the latest version) or a full `name=version` spec.")
- .long("dep")
- .short("d")
- .takes_value(true)
- .multiple(true)
- .requires("script")
- )
- .arg(Arg::with_name("dep_extern")
- .help("Like `dep`, except that it *also* adds a `#[macro_use] extern crate name;` item for expression and loop scripts. Note that this only works if the name of the dependency and the name of the library it generates are exactly the same.")
- .long("dep-extern")
- .short("D")
- .takes_value(true)
- .multiple(true)
- // .requires("script")
- .requires("expr_or_loop")
- )
- .arg(Arg::with_name("extern")
- .help("Adds an `#[macro_use] extern crate name;` item for expressions and loop scripts.")
- .long("extern")
- .short("x")
- .takes_value(true)
- .multiple(true)
- // .requires("script")
- .requires("expr_or_loop")
- )
- .arg(Arg::with_name("force")
- .help("Force the script to be rebuilt.")
- .long("force")
- .requires("script")
- )
- )
- .get_matches();
+ .bin_name("cargo")
+ .version(version)
+ .about(about)
+ .arg_required_else_help(true)
+ .subcommand_required_else_help(true)
+ .subcommand(SubCommand::with_name("script")
+ .version(version)
+ .about(about)
+ .usage("cargo script [FLAGS OPTIONS] [--] <script> <args>...")
+ .arg(Arg::with_name("script")
+ .help("Script file (with or without extension) to \
+ execute.")
+ .index(1))
+ .arg(Arg::with_name("args")
+ .help("Additional arguments passed to the script.")
+ .index(2)
+ .multiple(true))
+ .arg(Arg::with_name("expr")
+ .help("Execute <script> as a literal expression and \
+ display the result.")
+ .long("expr")
+ .short("e")
+ .conflicts_with_all(csas!["loop"])
+ .requires("script"))
+ .arg(Arg::with_name("loop")
+ .help("Execute <script> as a literal closure once for \
+ each line from stdin.")
+ .long("loop")
+ .short("l")
+ .conflicts_with_all(csas!["expr"])
+ .requires("script"))
+ .arg_group(ArgGroup::with_name("expr_or_loop")
+ .add_all(&["expr", "loop"]))
+ .arg(Arg::with_name("clear_cache")
+ .help("Clears out the script cache.")
+ .long("clear-cache"))
+ .arg(Arg::with_name("count")
+ .help("Invoke the loop closure with two arguments: \
+ line, and line number.")
+ .long("count")
+ .requires("loop"))
+ .arg(Arg::with_name("pkg_path")
+ .help("Specify where to place the generated Cargo \
+ package.")
+ .long("pkg-path")
+ .takes_value(true)
+ .requires("script")
+ .conflicts_with_all(csas!["clear_cache", "force"]))
+ .arg(Arg::with_name("gen_pkg_only")
+ .help("Generate the Cargo package, but don't compile \
+ or run it.")
+ .long("gen-pkg-only")
+ .requires("script")
+ .conflicts_with_all(csas!["args",
+ "build_only",
+ "debug",
+ "force"]))
+ .arg(Arg::with_name("build_only")
+ .help("Build the script, but don't run it.")
+ .long("build-only")
+ .requires("script")
+ .conflicts_with_all(csas!["args"]))
+ .arg(Arg::with_name("debug")
+ .help("Build a debug executable, not an optimised one.")
+ .long("debug")
+ .requires("script"))
+ .arg(Arg::with_name("dep")
+ .help("Add an additional Cargo dependency. Each SPEC \
+ can be either just the package name (which \
+ will assume the latest version) or a full \
+ `name=version` spec.")
+ .long("dep")
+ .short("d")
+ .takes_value(true)
+ .multiple(true)
+ .requires("script"))
+ .arg(Arg::with_name("dep_extern")
+ .help("Like `dep`, except that it *also* adds a \
+ `#[macro_use] extern crate name;` item for \
+ expression and loop scripts. Note that this \
+ only works if the name of the dependency and \
+ the name of the library it generates are \
+ exactly the same.")
+ .long("dep-extern")
+ .short("D")
+ .takes_value(true)
+ .multiple(true)
+ .requires("expr_or_loop"))
+ .arg(Arg::with_name("extern")
+ .help("Adds an `#[macro_use] extern crate name;` item \
+ for expressions and loop scripts.")
+ .long("extern")
+ .short("x")
+ .takes_value(true)
+ .multiple(true)
+ .requires("expr_or_loop"))
+ .arg(Arg::with_name("force")
+ .help("Force the script to be rebuilt.")
+ .long("force")
+ .requires("script")))
+ .get_matches();
let m = m.subcommand_matches("script").unwrap();Another example:
let prelude_items = {
- let dep_externs = args.dep_extern.iter()
- .map(|d| match d.find('=') {
- Some(i) => &d[..i],
- None => &d[..]
- })
- .map(|d| match d.contains('-') {
- true => Cow::from(d.replace("-", "_")),
- false => Cow::from(d)
- })
- .map(|d| format!("#[macro_use] extern crate {};", d));
-
- let externs = args.extern_.iter()
- .map(|n| format!("#[macro_use] extern crate {};", n));
+ let dep_externs = args.dep_extern
+ .iter()
+ .map(|d| {
+ match d.find('=') {
+ Some(i) => &d[..i],
+ None => &d[..],
+ }
+ })
+ .map(|d| {
+ match d.contains('-') {
+ true => Cow::from(d.replace("-", "_")),
+ false => Cow::from(d),
+ }
+ })
+ .map(|d| format!("#[macro_use] extern crate {};", d));
+
+ let externs = args.extern_
+ .iter()
+ .map(|n| format!("#[macro_use] extern crate {};", n));
let mut items: Vec<_> = dep_externs.chain(externs).collect();-
It deleted trailing commas from match arms that use
{…}blocks (#173). -
Actually,
rustfmtappears to be terrible at chained expressions, indenting like a crazy person. I triedchain_base_indent = "Tabbed", but it didn't appear to do anything.
script_name = path.file_stem()
- .map(|os| os.to_string_lossy().into_owned())
- .unwrap_or("unknown".into());
+ .map(|os| os.to_string_lossy().into_owned())
+ .unwrap_or("unknown".into());
let mut body = String::new();
try!(file.read_to_string(&mut body));- Ironically, it forces trailing commas for match arm expressions, even when they're completely redundant:
let dep = match dep.find('=') {
Some(_) => dep,
- None => dep + "=*"
+ None => dep + "=*",
};- It appears to be inconsistent about how long it'll allow a line to get. I mean, it broke this line when it did not break the longer line just after it:
let version = parts.next().expect("dependency is missing version");
- assert!(parts.next().is_none(), "dependency somehow has three parts?!");
+ assert!(parts.next().is_none(),
+ "dependency somehow has three parts?!");
if name == "" {
try!(Err((Blame::Human, "cannot have empty dependency package name")));
}Also note that fn_arg_indent = "Tabbed" doesn't appear to resolve the indentation on this, either.
rustfmtreally doesn't appear to like this; it's a shame because I do this quite frequently with "escape" branches:
- if path.is_file() { continue }
+ if path.is_file() {
+ continue;
+ }- Indentation of comments inside closures appears borked (#?):
let cleanup_dir: Defer<_, MainError> = Defer::defer(|| {
- // DO NOT try deleting ANYTHING if we're not cleaning up inside our own cache. We *DO NOT* want to risk killing user files.
+ // DO NOT try deleting ANYTHING if we're not cleaning up inside our own cache. We *DO NOT* want to risk killing user files.
if action.using_cache {
try!(fs::remove_dir_all(pkg_path))
}
Ok(())
});- ... actually, make that structs, too:
struct InputAction {
- /// Compile the input into a fresh executable?
+/// Compile the input into a fresh executable?
compile: bool,- Later in the same struct; It's actually deigned to convert a block doc comment... into a sequence of line comments that start with a spurious
*:
- /**
- Is the package directory in the cache?
+// *
+// Is the package directory in the cache?
+//
+// Currently, this can be inferred from `emit_metadata`, but there's no *intrinsic* reason they should be tied together.
+//
- Currently, this can be inferred from `emit_metadata`, but there's no *intrinsic* reason they should be tied together.
- */
using_cache: bool,- This is a good example of things I don't like. If you're scanning down the left of this
match, the default formatting makes it look like there's three arms instead of two. You have to look over and locate the end of the pattern to understand the code:
let (path, mtime) = match *input {
- Input::File(_, path, _, mtime)
- => (Some(path.to_string_lossy().into_owned()), Some(mtime)),
- Input::Expr(..)
- | Input::Loop(..)
- => (None, None)
+ Input::File(_, path, _, mtime) =>
+ (Some(path.to_string_lossy().into_owned()), Some(mtime)),
+ Input::Expr(..) |
+ Input::Loop(..) => (None, None),
};- You realise, of course, that this means war:
fn get_exe_path<P>(input: &Input, pkg_path: P, meta: &PackageMetadata) -> PathBuf
-where P: AsRef<Path> {
+ where P: AsRef<Path>
+{- More apparent inconsistency:
- let mut exe_path = pkg_path.as_ref().join("target").join(profile).join(&input.safe_name()).into_os_string();
+ let mut exe_path = pkg_path.as_ref()
+ .join("target")
+ .join(profile)
+ .join(&input.safe_name())
+ .into_os_string();vs
- let meta_str = try!(rustc_serialize::json::encode(meta)
- .map_err(|err| err.to_string()));
+ let meta_str = try!(rustc_serialize::json::encode(meta).map_err(|err| err.to_string()));This is bothersome when I'm inserting line breaks to divide up "logical" steps in a sequence of transforms.
-
Space around
=in associated type constraints is just hideous... I'm honestly not sure why, since I'd use spaces everywhere else. -
Another case of taking something simple and making it huge and ugly:
- id.push(if STUB_HASHES { "stub" } else { &*digest });
+ id.push(if STUB_HASHES {
+ "stub"
+ } else {
+ &*digest
+ });-
It outright stripped
TODOcomments out of the source. By default. That's a hanging offense, as far as I'm concerned. -
Another example of blatantly ridiculous rightward drift:
- const SPLIT_MARKERS: &'static [&'static str] = &[
- "//", "/*", "#![", "#[", "pub ",
- "extern ", "use ", "mod ", "type ",
- "struct ", "enum ", "fn ", "impl ", "impl<",
- "static ", "const ",
- ];
+ const SPLIT_MARKERS: &'static [&'static str] = &["//", "/*", "#![", "#[", "pub ",
+ "extern ", "use ", "mod ", "type ",
+ "struct ", "enum ", "fn ", "impl ",
+ "impl<", "static ", "const "];Setting the line width to 10,000 kind of solves this... it forces the whole literal on to a single line.
- I can guess why it changed this one, but it's still frustrating that this is now harder to read:
// To match librustdoc/html/markdown.rs, HOEDOWN_EXTENSIONS.
- let exts
- = hoedown::NO_INTRA_EMPHASIS
- | hoedown::TABLES
- | hoedown::FENCED_CODE
- | hoedown::AUTOLINK
- | hoedown::STRIKETHROUGH
- | hoedown::SUPERSCRIPT
- | hoedown::FOOTNOTES;
+ let exts = hoedown::NO_INTRA_EMPHASIS | hoedown::TABLES | hoedown::FENCED_CODE |
+ hoedown::AUTOLINK | hoedown::STRIKETHROUGH | hoedown::SUPERSCRIPT |
+ hoedown::FOOTNOTES;- It kills significant indentation in comments. You know what's funny? It did it to the comment explaining the code that does the exact same job in
cargo-script. :D
- /*
- On every line:
-
- - update nesting level and detect end-of-comment
- - if margin is None:
- - if there appears to be a margin, set margin.
- - strip off margin marker
- - update the leading space counter
- - strip leading space
- - append content
- */
+ //
+ // On every line:
+ //
+ // - update nesting level and detect end-of-comment
+ // - if margin is None:
+ // - if there appears to be a margin, set margin.
+ // - strip off margin marker
+ // - update the leading space counter
+ // - strip leading space
+ // - append content
+ //
+