Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Commit

Permalink
Bad Rustfmt formattings
Browse files Browse the repository at this point in the history
  • Loading branch information
killercup committed Mar 28, 2017
1 parent a3a9ec4 commit e9758f6
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 69 deletions.
5 changes: 5 additions & 0 deletions rustfmt.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn_args_layout = "Block"
array_layout = "Block"
where_style = "Rfc"
generics_indent = "Block"
fn_call_style = "Block"
8 changes: 5 additions & 3 deletions waltz/src/code_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl CodeBlock {
let path = Path::new(root.as_ref()).join(&self.filename());
let parent = match path.parent() {
Some(p) => p,
None => bail!("Can't create file for code block, path has no parent directory"),
None => bail!("Can't create file for code block, path has no parent directory",),

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

That seems just wrong

This comment has been minimized.

Copy link
@nrc
};

create_dir_all(parent)?;
Expand All @@ -82,7 +82,8 @@ mod test {

#[test]
fn parsing() {
let example = unindent(r#"
let example = unindent(
r#"

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

Argument list with one argument that happens to be multiple lines

This comment has been minimized.

Copy link
@nrc

nrc Mar 28, 2017

Part of the discussion in rust-lang/style-team#65

# Lorem ipsum
## Shell
Expand All @@ -98,7 +99,8 @@ mod test {
println!("Dolor sit amet");
}
```
"#);
"#,

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

Even assuming I actually wanted this to look like an argument list, this is not at the right indent level

This comment has been minimized.

Copy link
@nrc

nrc Mar 28, 2017

Rustfmt will not change anything inside a string, so you get the old indent level. If you fixed this manually, rustfmt won't break it again. We might fix this in a long run, but it is a huge can of worms

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

Ah, that makes sense. Still does not look pretty, though :(

This comment has been minimized.

Copy link
@solson

solson Mar 28, 2017

A way to give rustfmt more flexibility with strings is to use the \<whitespace> syntax, e.g. like https://is.gd/jwVNnt. Unfortunately, this doesn't work at all in raw strings.

);

let markdown = ::pulldown_cmark::Parser::new(&example);
let code_blocks = ::extract_code_blocks(markdown).unwrap();
Expand Down
27 changes: 15 additions & 12 deletions waltz_cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,22 @@ pub fn app() -> App<'static, 'static> {
.version(crate_version!())
.author("Pascal Hertleif <killercup@gmail.com>")
.about("Extract code blocks from Markdown and save them as files.")
.arg(Arg::with_name("input_file")
.help("The input markdown file")
.index(1)
.required(true)
.arg(
Arg::with_name("input_file")
.help("The input markdown file")
.index(1)
.required(true),

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

Same thing as above, one parameter that is multiple lines long.

)
.arg(Arg::with_name("target_dir")
.help("The target directory")
.short("o")
.takes_value(true)
.arg(
Arg::with_name("target_dir")
.help("The target directory")
.short("o")
.takes_value(true),
)
.arg(Arg::with_name("v")
.short("v")
.help("Enable logging, use multiple `v`s to increase verbosity")
.multiple(true)
.arg(
Arg::with_name("v")
.short("v")
.help("Enable logging, use multiple `v`s to increase verbosity")
.multiple(true),
)
}
9 changes: 6 additions & 3 deletions waltz_cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ fn try_main() -> Result<()> {
// Parse markdown file
let input = {
let mut res = String::new();
let mut f = File::open(input_file)
.chain_err(|| format!("Error opening file `{}`", input_file))?;
let mut f =
File::open(input_file).chain_err(|| format!("Error opening file `{}`", input_file))?;

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

I'm not sure about this, actually. But not putting the method call on a new line is definitely weird.

f.read_to_string(&mut res)
.chain_err(|| format!("Error reading file `{}`", input_file))?;
info!("Read file `{}`", input_file);
Expand All @@ -48,7 +48,10 @@ fn try_main() -> Result<()> {

let code_blocks = waltz::extract_code_blocks(parser)?;

info!("Found {} code blocks (not all might have file names)", code_blocks.len());
info!(
"Found {} code blocks (not all might have file names)",
code_blocks.len(),
);

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

Is this a special thing for format_args macros (which would be okay) or the same weird argument list thing from above? Still a potential bug hazard as not all macros support trailing commas!

This comment has been minimized.

Copy link
@nrc

nrc Mar 28, 2017

I'm not sure what 'weird argument list thing' is. () macros which parse as an expression list are formatted like function calls. Also, rust-lang/rustfmt#1419


// Output files
for code_block in code_blocks.iter().filter(|cb| cb.has_filename()) {
Expand Down
26 changes: 18 additions & 8 deletions waltz_cli/tests/concat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ use utils::*;

#[test]
fn concat() {
given(r#"
given(
r#"
# Getting started
First of all, create a simple `Cargo.toml` file:
Expand Down Expand Up @@ -45,15 +46,22 @@ fn concat() {
}
}
```
"#)
.running(waltz)
.creates(file("Cargo.toml").containing(r#"
"#,
)
.running(waltz)

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

This a method call in a chain that started with a function call, not a let declaration or anything. It should be on the same ident level. That vertical space between the closing bracket and the .running looks really weird.

(Also that multi line arg thing from above, of course)

This comment has been minimized.

.creates(
file("Cargo.toml").containing(
r#"
[package]
authors = ["Pascal Hertleif <killercup@gmail.com>"]
name = "foo"
version = "0.1.0"
"#))
.creates(file("src/lib.rs").containing(r#"
"#,
),
)
.creates(
file("src/lib.rs").containing(
r#"
struct Foo {
x: i32,
}
Expand All @@ -69,6 +77,8 @@ fn concat() {
Foo { x: 42 }
}
}
"#))
.running(cargo_check);
"#,
),
)
.running(cargo_check);
}
42 changes: 26 additions & 16 deletions waltz_cli/tests/simple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ use utils::*;

#[test]
fn simple() {
given(r#"
given(
r#"
# Getting started
First of all, create a simple `Cargo.toml` file:
Expand All @@ -22,25 +23,35 @@ fn simple() {
println!("Hello, world!");
}
```
"#)
.running(waltz)
.creates(file("Cargo.toml").containing(r#"
"#,
)
.running(waltz)
.creates(
file("Cargo.toml").containing(
r#"
[package]
authors = ["Pascal Hertleif <killercup@gmail.com>"]
name = "foo"
version = "0.1.0"
"#))
.creates(file("src/main.rs").containing(r#"
"#,
),
)
.creates(
file("src/main.rs").containing(
r#"
fn main() {
println!("Hello, world!");
}
"#))
.running(|cwd| main(cwd).prints("Hello, world!"));
"#,
),
)
.running(|cwd| main(cwd).prints("Hello, world!"));
}

#[test]
fn complex_paths() {
given(r#"
given(
r#"
First off:
```toml,file=Cargo.toml
Expand All @@ -61,11 +72,10 @@ fn complex_paths() {
println!("Sup dawg I herd u likd nested dirs");
}
```
"#)
.running(waltz)
.creates(file("Cargo.toml"))
.creates(file("src/bin/lolwut/main.rs"))
.running(|cwd| binary(cwd, "lolwut")
.prints("Sup dawg")
);
"#,
)
.running(waltz)
.creates(file("Cargo.toml"))
.creates(file("src/bin/lolwut/main.rs"))
.running(|cwd| binary(cwd, "lolwut").prints("Sup dawg"));
}
49 changes: 22 additions & 27 deletions waltz_cli/tests/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,34 +25,29 @@ pub fn file(path: &str) -> FileAssert {

pub fn waltz(cwd: &Path) -> CliAssert {
CliAssert::main_binary()
.with_args(&[
"-vvv",
cwd.join("test.md").to_str().unwrap(),
"-o", cwd.to_str().unwrap(),
])
.with_args(&["-vvv", cwd.join("test.md").to_str().unwrap(), "-o", cwd.to_str().unwrap()],)

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

I'm sure there a simple heuristic that can differentiate between arbitrary and deliberate argument/list item groupings with enough confidence that rustfmt can decide to skip potentially deliberate groupings. Maybe just for ones that don't have shitty formatting (like too long lines).

This comment has been minimized.

Copy link
@nrc

nrc Mar 28, 2017

rust-lang/rustfmt#1421

I don't think we'll group like you want to - I found trying to take into account user formatting caused a lot of problems, but we could at least not put complex arrays like this on one line

.succeeds()
}

pub fn main(cwd: &Path) -> CliAssert {
CliAssert::command(&[
"cargo", "run",
"--manifest-path", cwd.join("Cargo.toml").to_str().unwrap(),
])
CliAssert::command(&["cargo", "run", "--manifest-path", cwd.join("Cargo.toml").to_str().unwrap()],)
}

pub fn binary(cwd: &Path, name: &str) -> CliAssert {
CliAssert::command(&[
"cargo", "run",
"--manifest-path", cwd.join("Cargo.toml").to_str().unwrap(),
"--bin", name,
])
CliAssert::command(
&[
"cargo",
"run",
"--manifest-path",
cwd.join("Cargo.toml").to_str().unwrap(),
"--bin",
name,
],

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

…otherwise it leads to stuff like this here. I'm okay with automatic formatting with multiple styles, but I'd really like consistency in a file.

)
}

fn cargo(cwd: &Path, subcommand: &str) -> CliAssert {
CliAssert::command(&[
"cargo", subcommand,
"--manifest-path", cwd.join("Cargo.toml").to_str().unwrap(),
])
CliAssert::command(&["cargo", subcommand, "--manifest-path", cwd.join("Cargo.toml").to_str().unwrap()],)
}

pub fn cargo_check(cwd: &Path) -> CliAssert {
Expand Down Expand Up @@ -85,27 +80,25 @@ impl Assert {
fn with_file(content: &str) -> Self {
let a = Assert::default();

create_dir_all(&a.output_dir)
.expect("error creating output dir");
create_dir_all(&a.output_dir).expect("error creating output dir");

let mut f = File::create(a.output_dir.join("test.md"))
.expect("error create md file");
let mut f = File::create(a.output_dir.join("test.md")).expect("error create md file");
f.write_all(unindent(content).as_bytes())
.expect("error writing md file");

a
}

pub fn running<F>(&self, cmd: F) -> &Self where
pub fn running<F>(&self, cmd: F) -> &Self
where

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

I'm surprisingly fine with this change, but still think the previous style is nicer.

F: for<'cwd> Fn(&'cwd Path) -> CliAssert,
{
cmd(&self.output_dir).unwrap();
self
}

pub fn creates(&self, fa: FileAssert) -> &Self {
fa.context(self.output_dir.to_owned())
.unwrap();
fa.context(self.output_dir.to_owned()).unwrap();
self
}
}
Expand Down Expand Up @@ -137,14 +130,16 @@ impl FileAssert {
}

fn unwrap(self) {
let dir = self.working_dir.expect(&format!("No working dir set for `{}`", self.path));
let dir = self.working_dir
.expect(&format!("No working dir set for `{}`", self.path));
let path = PathBuf::from(dir).join(&self.path);

let mut f = File::open(&path).expect(&format!("no file at {:?}", path));

if let Some(expected_content) = self.content {
let mut content = String::new();
f.read_to_string(&mut content).expect(&format!("failed to read {:?}", path));
f.read_to_string(&mut content)
.expect(&format!("failed to read {:?}", path));

This comment has been minimized.

Copy link
@killercup

killercup Mar 28, 2017

Author Owner

These last two are more cases of perceived inconsistency. Above, it put expect and unwrap on the same line, but here it chose not to. I can see why (chains with > 2 items), but it seems inconsistent.

This comment has been minimized.

Copy link
@nrc

nrc Mar 28, 2017

So there is a heuristic for maximum chars in a chain before they are split on to multiple lines. That usually works quite well, but it can lead to unexpected splits like these two sometimes. I think this is just somewhere where we can't be perfect 100% of the time.


let diff = Changeset::new(&content, &unindent(&expected_content), "\n");
if diff.distance > 0 {
Expand Down

0 comments on commit e9758f6

Please sign in to comment.