This repository has been archived by the owner on Dec 29, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 21
Review/feedback #16
Merged
Merged
Review/feedback #16
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5681d0b
cargo.toml keywords
colin-kiegel ed19a66
README.md
colin-kiegel cc2d7be
documentation tweaks
colin-kiegel cdf0d1e
error formatting
colin-kiegel 0ee479c
refactor output assertion
colin-kiegel fcd20f1
refactor output assertion II
colin-kiegel 1f29f4f
allow matching without asserting the exit status
colin-kiegel f45db85
refactor `assert_cmd!` macro
colin-kiegel 8a01125
expect success by default + cleanup
colin-kiegel e307b80
some doc changes
colin-kiegel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way you split out this struct, it's very neat.
Seeing how the code below with the matches on fuzziness etc. is pretty much duplicated, I'm wondering if maybe it can become a method on this type, like
OutputAssertion::compare_with(String)
? (It might also be a good idea to make this struct generic over a traitOutputType
, but more on that in a comment below.)At one point I was thinking about introducing an
enum Precision { Exact, Fuzzy }
instead of using a bool, but didn't go through with it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we start a separate discussion about future refactoring, e.g. in the ticket #20 you created, or yet another general architecture discussion ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think I'll just open a new PR where I try that design after this one gets merged. Issues that are not about bugs or features but contain a ton of quoted code are kinda hard to keep track of :)