fix(output): Re-work API to work with rustfmt #74

epage · 2017-10-28T18:05:59Z

rustfmt will split long builers across lines, even when that breaks
logical grouping.

For example

assert_cli::Assert::command(&["ls", "foo-bar-foo"])
    .fails()
    .and()
    .stderr().contains("foo-bar-foo")
    .unwrap();

will be turned into

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.

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 #70

BREAKING CHANGE: .stdout().contains(text) is now
.stdout(assert_cli::Output::contains(text), etc.

I have created tests for any new feature, or added regression tests for bugfixes.
cargo test succeeds
Clippy is happy: cargo +nightly clippy succeeds
Rustfmt is happy: cargo +nightly fmt succeeds

epage · 2017-10-28T18:09:21Z

I can take or leave the API change.

This did push me to making the code more composable which is nice.

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

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.

killercup · 2017-10-29T10:45:27Z

README.md

@@ -37,20 +37,20 @@ fn main() {
    assert_cli::Assert::command(&["ls", "foo-bar-foo"])
        .fails()
        .and()
-        .stderr().contains("foo-bar-foo")
+        .stderr(assert_cli::Output::contains("foo-bar-foo"))


I'd probably use assert_cli::{Assert, Output}; in most examples.

killercup · 2017-10-29T10:51:12Z

src/output.rs

        if result != self.expected_result {
            if self.expected_result {
-                bail!(ErrorKind::OutputDoesntContain(
+                let nice_diff = diff::render(&differences)?;


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

killercup · 2017-10-29T11:01:19Z

src/output.rs

+            expected_result: false,
+        };
+        Self::new(StrPredicate::Is(pred))
+    }


After reading for of these I fell the need to write a macro for this… which would probably not make the code much clearer 😅

killercup · 2017-10-29T11:26:33Z

src/assert.rs

-            kind: OutputKind::StdErr,
-            expected_result: true,
-        }
+    pub fn stderr(mut self, pred: Output) -> Self {


We'll need to document that you can call this method multiple times

Originally based on #74

killercup · 2018-01-11T08:39:44Z

Rustfmt is happy

Sorry this discussion has gone stale. Just wanted to ask here: Were there any rustfmt updates that changed this since October?

epage · 2018-01-12T01:25:23Z

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.

epage · 2018-05-29T12:33:39Z

Addressed in https://github.com/assert-rs/assert_cmd

epage force-pushed the feat/out branch 2 times, most recently from c760d80 to 3afd1e6 Compare October 28, 2017 18:21

epage force-pushed the feat/out branch from 3afd1e6 to 063b3ce Compare October 28, 2017 18:29

This was referenced Oct 28, 2017

Output satisfies predicate? #55

Closed

[WIP] Add support for arbitrary predicates #57

Closed

killercup reviewed Oct 29, 2017

View reviewed changes

killercup added a commit that referenced this pull request Oct 29, 2017

Free fns for Output constructors and add prelude

491d648

Originally based on #74

killercup mentioned this pull request Oct 29, 2017

WIP: Free fns for Output constructors and add prelude #75

Open

1 task

This was referenced Nov 3, 2017

Feature: Or-Operator #19

Closed

Provide option to not trim output strings #77

Closed

epage mentioned this pull request Jan 25, 2018

Regex matching. #85

Closed

4 tasks

epage closed this Feb 2, 2018

epage deleted the feat/out branch February 2, 2018 23:58

epage restored the feat/out branch February 2, 2018 23:58

epage reopened this Feb 3, 2018

epage mentioned this pull request Feb 3, 2018

Refactor output #87

Merged

epage closed this May 29, 2018

epage deleted the feat/out branch May 29, 2018 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(output): Re-work API to work with rustfmt #74

fix(output): Re-work API to work with rustfmt #74

epage commented Oct 28, 2017

epage commented Oct 28, 2017

killercup left a comment

killercup Oct 29, 2017

killercup Oct 29, 2017

killercup Oct 29, 2017

killercup Oct 29, 2017

killercup commented Jan 11, 2018

epage commented Jan 12, 2018

epage commented May 29, 2018

fix(output): Re-work API to work with rustfmt #74

fix(output): Re-work API to work with rustfmt #74

Conversation

epage commented Oct 28, 2017

epage commented Oct 28, 2017

killercup left a comment

Choose a reason for hiding this comment

killercup Oct 29, 2017

Choose a reason for hiding this comment

killercup Oct 29, 2017

Choose a reason for hiding this comment

killercup Oct 29, 2017

Choose a reason for hiding this comment

killercup Oct 29, 2017

Choose a reason for hiding this comment

killercup commented Jan 11, 2018

epage commented Jan 12, 2018

epage commented May 29, 2018