fix(output): Re-work API to work with rustfmt#74
fix(output): Re-work API to work with rustfmt#74epage wants to merge 1 commit intoassert-rs:masterfrom
Conversation
|
I can take or leave the API change. This did push me to making the code more composable which is nice. |
epage
force-pushed
the
feat/out
branch
2 times, most recently
from
c760d80 to
3afd1e6
Compare
rustfmt will split long builers across lines, even when that breaks
logical grouping.
For example
```rust
assert_cli::Assert::command(&["ls", "foo-bar-foo"])
.fails()
.and()
.stderr().contains("foo-bar-foo")
.unwrap();
```
will be turned into
```rust
assert_cli::Assert::command(&["ls", "foo-bar-foo"])
.fails()
.and()
.stderr()
.contains("foo-bar-foo")
.unwrap();
```
which obscures intent.
Normally, I don't like working around tools but this one seems
sufficient to do so.
```rust
assert_cli::Assert::command(&["ls", "foo-bar-foo"])
.fails()
.and()
.stderr(assert_cli::Output::contains("foo-bar-foo"))
.unwrap();
```
Pros
- More consistent with `with_env`
- Can add support for accepting arrays
- Still auto-complete / docs friendly
- Still expandable to additional assertions without much duplication or
losing out on good error reporting
Cons
- More verbose if you don't `use assert_cli::{Assert, Environment, Output}`
Alternatives
- Accept distinct predicates
- e.g. `.stderr(assert_cli::Is::text("foo-bar-foo"))`
- e.g. `.stderr(assert_cli::Is::not("foo-bar-foo"))`
- Strange `text` function
- More structs to `use`
- Less auto-complete / docs friendly (lacks contextual discovery or
whatever the UX term is)
Fixes assert-rs#70
BREAKING CHANGE: `.stdout().contains(text)` is now
`.stdout(assert_cli::Output::contains(text)`, etc.
killercup
left a comment
killercup
left a comment
There was a problem hiding this comment.
I like this! I feel like we are slowly moving to a structure of fractal asserts. We should try to structure (Cli)Assert, Environment(Assert), and Output(Assert) in a similar way.
More verbose if you don't use assert_cli::{Assert, Environment, Output}
Yes, that's not really nice. But I have an idea. Let me whip up a PR based on this one with free standing functions instead of constructors on Output and a prelude module.
| .fails() | ||
| .and() | ||
| .stderr().contains("foo-bar-foo") | ||
| .stderr(assert_cli::Output::contains("foo-bar-foo")) |
There was a problem hiding this comment.
I'd probably use assert_cli::{Assert, Output}; in most examples.
| if result != self.expected_result { | ||
| if self.expected_result { | ||
| bail!(ErrorKind::OutputDoesntContain( | ||
| let nice_diff = diff::render(&differences)?; |
There was a problem hiding this comment.
Do we really want to early-return this error? I think I'd .unwrap_or_else(|e| format!("(unable to render nice diff, error was: {:?})", e)) instead of ? here and give the user both the interesting error as well as the diff-error in one go
| expected_result: false, | ||
| }; | ||
| Self::new(StrPredicate::Is(pred)) | ||
| } |
There was a problem hiding this comment.
After reading for of these I fell the need to write a macro for this… which would probably not make the code much clearer 😅
| kind: OutputKind::StdErr, | ||
| expected_result: true, | ||
| } | ||
| pub fn stderr(mut self, pred: Output) -> Self { |
There was a problem hiding this comment.
We'll need to document that you can call this method multiple times
Sorry this discussion has gone stale. Just wanted to ask here: Were there any rustfmt updates that changed this since October? |
Unsure. I'll need to check that when I go and take care of the rest of your feedback. I held off on all of it because I didn't want to do a lot of churn if we decided to go with #75 instead. |
|
Addressed in https://github.com/assert-rs/assert_cmd |
2 participants
rustfmt will split long builers across lines, even when that breaks
logical grouping.
For example
will be turned into
which obscures intent.
Normally, I don't like working around tools but this one seems
sufficient to do so.
Pros
with_envlosing out on good error reporting
Cons
use assert_cli::{Assert, Environment, Output}Alternatives
.stderr(assert_cli::Is::text("foo-bar-foo")).stderr(assert_cli::Is::not("foo-bar-foo"))textfunctionusewhatever the UX term is)
Fixes #70
BREAKING CHANGE:
.stdout().contains(text)is now.stdout(assert_cli::Output::contains(text), etc.cargo testsucceedscargo +nightly clippysucceedscargo +nightly fmtsucceeds