Skip to content

Commit b11124e

Browse files
hovinenhovinen
committed
Change Matcher::describe and Matcher::explain_match to return Description rather than String.
Previously, the description and match explanation were rendered greedily as a `String`. This meant that each matcher was responsible for making its own local decisions about how its content was rendered, such as whether and where newlines should be placed. But each matcher does not have enough context to make such decisions, leading to various corner cases where rendering was suboptimal. This will allow the rendering of the description to be controlled more precisely once all of its components are known. This change only changes the return values of the above methods and of certain helper methods. It should be effectively a no-op, since it just packs the existing rendered `String`'s into `Description` structs. This also introduces a couple of trait implementations for `Description` to simplify construction of a `Description` from strings as well as from sequences of other `Description`. Future commits will change how the `Description` are constructed to take advantage of the structure now being communicated upwards.
1 parent c0a1158 commit b11124e

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

44 files changed

+451
-310
lines changed

‎googletest/crate_docs.md‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ a struct holding the matcher's data and have it implement the trait
209209
[`Matcher`]:
210210

211211
```no_run
212-
use googletest::matcher::{Matcher, MatcherResult};
212+
use googletest::{description::Description, matcher::{Matcher, MatcherResult}};
213213
use std::fmt::Debug;
214214
215215
struct MyEqMatcher<T> {
@@ -227,13 +227,13 @@ impl<T: PartialEq + Debug> Matcher for MyEqMatcher<T> {
227227
}
228228
}
229229
230-
fn describe(&self, matcher_result: MatcherResult) -> String {
230+
fn describe(&self, matcher_result: MatcherResult) -> Description {
231231
match matcher_result {
232232
MatcherResult::Match => {
233-
format!("is equal to {:?} the way I define it", self.expected)
233+
format!("is equal to {:?} the way I define it", self.expected).into()
234234
}
235235
MatcherResult::NoMatch => {
236-
format!("isn't equal to {:?} the way I define it", self.expected)
236+
format!("isn't equal to {:?} the way I define it", self.expected).into()
237237
}
238238
}
239239
}
@@ -243,7 +243,7 @@ impl<T: PartialEq + Debug> Matcher for MyEqMatcher<T> {
243243
It is recommended to expose a function which constructs the matcher:
244244

245245
```no_run
246-
# use googletest::matcher::{Matcher, MatcherResult};
246+
# use googletest::{description::Description, matcher::{Matcher, MatcherResult}};
247247
# use std::fmt::Debug;
248248
#
249249
# struct MyEqMatcher<T> {
@@ -261,13 +261,13 @@ impl<T: PartialEq + Debug> Matcher for MyEqMatcher<T> {
261261
# }
262262
# }
263263
#
264-
# fn describe(&self, matcher_result: MatcherResult) -> String {
264+
# fn describe(&self, matcher_result: MatcherResult) -> Description {
265265
# match matcher_result {
266266
# MatcherResult::Match => {
267-
# format!("is equal to {:?} the way I define it", self.expected)
267+
# format!("is equal to {:?} the way I define it", self.expected).into()
268268
# }
269269
# MatcherResult::NoMatch => {
270-
# format!("isn't equal to {:?} the way I define it", self.expected)
270+
# format!("isn't equal to {:?} the way I define it", self.expected).into()
271271
# }
272272
# }
273273
# }
@@ -282,7 +282,7 @@ impl<T: PartialEq + Debug> Matcher for MyEqMatcher<T> {
282282

283283
```
284284
# use googletest::prelude::*;
285-
# use googletest::matcher::{Matcher, MatcherResult};
285+
# use googletest::{description::Description, matcher::{Matcher, MatcherResult}};
286286
# use std::fmt::Debug;
287287
#
288288
# struct MyEqMatcher<T> {
@@ -300,13 +300,13 @@ impl<T: PartialEq + Debug> Matcher for MyEqMatcher<T> {
300300
# }
301301
# }
302302
#
303-
# fn describe(&self, matcher_result: MatcherResult) -> String {
303+
# fn describe(&self, matcher_result: MatcherResult) -> Description {
304304
# match matcher_result {
305305
# MatcherResult::Match => {
306-
# format!("is equal to {:?} the way I define it", self.expected)
306+
# format!("is equal to {:?} the way I define it", self.expected).into()
307307
# }
308308
# MatcherResult::NoMatch => {
309-
# format!("isn't equal to {:?} the way I define it", self.expected)
309+
# format!("isn't equal to {:?} the way I define it", self.expected).into()
310310
# }
311311
# }
312312
# }

‎googletest/src/description.rs‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,22 +41,24 @@ use std::fmt::{Display, Formatter, Result};
4141
/// They can also be indented, enumerated and or
4242
/// bullet listed if [`Description::indent`], [`Description::enumerate`], or
4343
/// respectively [`Description::bullet_list`] has been called.
44-
#[derive(Debug)]
44+
#[derive(Debug, Default)]
4545
pub struct Description {
4646
elements: Vec<String>,
4747
indent_mode: IndentMode,
4848
list_style: ListStyle,
4949
}
5050

51-
#[derive(Debug)]
51+
#[derive(Debug, Default)]
5252
enum IndentMode {
53+
#[default]
5354
NoIndent,
5455
EveryLine,
5556
AllExceptFirstLine,
5657
}
5758

58-
#[derive(Debug)]
59+
#[derive(Debug, Default)]
5960
enum ListStyle {
61+
#[default]
6062
NoList,
6163
Bullet,
6264
Enumerate,
@@ -229,6 +231,9 @@ impl Display for Description {
229231
writeln!(f)?;
230232
write!(f, "{:other_line_indent$}{line}", "")?;
231233
}
234+
if element.ends_with("\n") {
235+
writeln!(f)?;
236+
}
232237
first_line_indent = first_line_of_element_indent;
233238
}
234239
Ok(())
@@ -240,11 +245,22 @@ impl FromIterator<String> for Description {
240245
where
241246
T: IntoIterator<Item = String>,
242247
{
243-
Self {
244-
elements: iter.into_iter().collect(),
245-
indent_mode: IndentMode::NoIndent,
246-
list_style: ListStyle::NoList,
247-
}
248+
Self { elements: iter.into_iter().collect(), ..Default::default() }
249+
}
250+
}
251+
252+
impl FromIterator<Description> for Description {
253+
fn from_iter<T>(iter: T) -> Self
254+
where
255+
T: IntoIterator<Item = Description>,
256+
{
257+
Self { elements: iter.into_iter().map(|s| format!("{s}")).collect(), ..Default::default() }
258+
}
259+
}
260+
261+
impl<T: Into<String>> From<T> for Description {
262+
fn from(value: T) -> Self {
263+
Self { elements: vec![value.into()], ..Default::default() }
248264
}
249265
}
250266

‎googletest/src/matcher.rs‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
//! The components required to implement matchers.
1616
17+
use crate::description::Description;
1718
use crate::internal::source_location::SourceLocation;
1819
use crate::internal::test_outcome::TestAssertionFailure;
1920
use crate::matchers::__internal_unstable_do_not_depend_on_these::ConjunctionMatcher;
@@ -57,7 +58,7 @@ pub trait Matcher {
5758
/// [`some`][crate::matchers::some] implements `describe` as follows:
5859
///
5960
/// ```ignore
60-
/// fn describe(&self, matcher_result: MatcherResult) -> String {
61+
/// fn describe(&self, matcher_result: MatcherResult) -> Description {
6162
/// match matcher_result {
6263
/// MatcherResult::Matches => {
6364
/// format!("has a value which {}", self.inner.describe(MatcherResult::Matches))
@@ -75,7 +76,7 @@ pub trait Matcher {
7576
/// output of `explain_match` is always used adjectivally to describe the
7677
/// actual value, while `describe` is used in contexts where a relative
7778
/// clause would not make sense.
78-
fn describe(&self, matcher_result: MatcherResult) -> String;
79+
fn describe(&self, matcher_result: MatcherResult) -> Description;
7980

8081
/// Prepares a [`String`] describing how the expected value
8182
/// encoded in this instance matches or does not match the given value
@@ -114,7 +115,7 @@ pub trait Matcher {
114115
/// inner matcher and appears as follows:
115116
///
116117
/// ```ignore
117-
/// fn explain_match(&self, actual: &Self::ActualT) -> String {
118+
/// fn explain_match(&self, actual: &Self::ActualT) -> Description {
118119
/// self.expected.explain_match(actual.deref())
119120
/// }
120121
/// ```
@@ -124,12 +125,12 @@ pub trait Matcher {
124125
/// inner matcher at a point where a relative clause would fit. For example:
125126
///
126127
/// ```ignore
127-
/// fn explain_match(&self, actual: &Self::ActualT) -> String {
128+
/// fn explain_match(&self, actual: &Self::ActualT) -> Description {
128129
/// format!("which points to a value {}", self.expected.explain_match(actual.deref()))
129130
/// }
130131
/// ```
131-
fn explain_match(&self, actual: &Self::ActualT) -> String {
132-
format!("which {}", self.describe(self.matches(actual)))
132+
fn explain_match(&self, actual: &Self::ActualT) -> Description {
133+
format!("which {}", self.describe(self.matches(actual))).into()
133134
}
134135

135136
/// Constructs a matcher that matches both `self` and `right`.
@@ -240,7 +241,11 @@ pub enum MatcherResult {
240241

241242
impl From<bool> for MatcherResult {
242243
fn from(b: bool) -> Self {
243-
if b { MatcherResult::Match } else { MatcherResult::NoMatch }
244+
if b {
245+
MatcherResult::Match
246+
} else {
247+
MatcherResult::NoMatch
248+
}
244249
}
245250
}
246251

‎googletest/src/matchers/all_matcher.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ pub mod internal {
102102
MatcherResult::Match
103103
}
104104

105-
fn explain_match(&self, actual: &Self::ActualT) -> String {
105+
fn explain_match(&self, actual: &Self::ActualT) -> Description {
106106
match N {
107107
0 => anything::<T>().explain_match(actual),
108108
1 => self.components[0].explain_match(actual),
@@ -114,15 +114,15 @@ pub mod internal {
114114
.map(|component| component.explain_match(actual))
115115
.collect::<Description>();
116116
if failures.len() == 1 {
117-
format!("{}", failures)
117+
format!("{}", failures).into()
118118
} else {
119-
format!("{}", failures.bullet_list().indent_except_first_line())
119+
format!("{}", failures.bullet_list().indent_except_first_line()).into()
120120
}
121121
}
122122
}
123123
}
124124

125-
fn describe(&self, matcher_result: MatcherResult) -> String {
125+
fn describe(&self, matcher_result: MatcherResult) -> Description {
126126
match N {
127127
0 => anything::<T>().describe(matcher_result),
128128
1 => self.components[0].describe(matcher_result),
@@ -142,6 +142,7 @@ pub mod internal {
142142
"has at least one of the following properties"
143143
}
144144
)
145+
.into()
145146
}
146147
}
147148
}

‎googletest/src/matchers/any_matcher.rs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ pub mod internal {
9696
MatcherResult::from(self.components.iter().any(|c| c.matches(actual).is_match()))
9797
}
9898

99-
fn explain_match(&self, actual: &Self::ActualT) -> String {
99+
fn explain_match(&self, actual: &Self::ActualT) -> Description {
100100
match N {
101-
0 => format!("which {}", anything::<T>().describe(MatcherResult::NoMatch)),
101+
0 => format!("which {}", anything::<T>().describe(MatcherResult::NoMatch)).into(),
102102
1 => self.components[0].explain_match(actual),
103103
_ => {
104104
let failures = self
@@ -108,15 +108,15 @@ pub mod internal {
108108
.map(|component| component.explain_match(actual))
109109
.collect::<Description>();
110110
if failures.len() == 1 {
111-
format!("{}", failures)
111+
format!("{}", failures).into()
112112
} else {
113-
format!("{}", failures.bullet_list().indent_except_first_line())
113+
format!("{}", failures.bullet_list().indent_except_first_line()).into()
114114
}
115115
}
116116
}
117117
}
118118

119-
fn describe(&self, matcher_result: MatcherResult) -> String {
119+
fn describe(&self, matcher_result: MatcherResult) -> Description {
120120
match N {
121121
0 => anything::<T>().describe(matcher_result),
122122
1 => self.components[0].describe(matcher_result),
@@ -136,6 +136,7 @@ pub mod internal {
136136
"has none of the following properties"
137137
}
138138
)
139+
.into()
139140
}
140141
}
141142
}

‎googletest/src/matchers/anything_matcher.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use crate::matcher::{Matcher, MatcherResult};
15+
use crate::{
16+
description::Description,
17+
matcher::{Matcher, MatcherResult},
18+
};
1619
use std::{fmt::Debug, marker::PhantomData};
1720

1821
/// Matches anything. This matcher always succeeds.
@@ -42,10 +45,10 @@ impl<T: Debug + ?Sized> Matcher for Anything<T> {
4245
MatcherResult::Match
4346
}
4447

45-
fn describe(&self, matcher_result: MatcherResult) -> String {
48+
fn describe(&self, matcher_result: MatcherResult) -> Description {
4649
match matcher_result {
47-
MatcherResult::Match => "is anything".to_string(),
48-
MatcherResult::NoMatch => "never matches".to_string(),
50+
MatcherResult::Match => "is anything".into(),
51+
MatcherResult::NoMatch => "never matches".into(),
4952
}
5053
}
5154
}

‎googletest/src/matchers/char_count_matcher.rs‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use crate::matcher::{Matcher, MatcherResult};
15+
use crate::{
16+
description::Description,
17+
matcher::{Matcher, MatcherResult},
18+
};
1619
use std::{fmt::Debug, marker::PhantomData};
1720

1821
/// Matches a string whose number of Unicode scalars matches `expected`.
@@ -71,36 +74,36 @@ impl<T: Debug + ?Sized + AsRef<str>, E: Matcher<ActualT = usize>> Matcher for Ch
7174
self.expected.matches(&actual.as_ref().chars().count())
7275
}
7376

74-
fn describe(&self, matcher_result: MatcherResult) -> String {
77+
fn describe(&self, matcher_result: MatcherResult) -> Description {
7578
match matcher_result {
76-
MatcherResult::Match => {
77-
format!(
78-
"has character count, which {}",
79-
self.expected.describe(MatcherResult::Match)
80-
)
81-
}
82-
MatcherResult::NoMatch => {
83-
format!(
84-
"has character count, which {}",
85-
self.expected.describe(MatcherResult::NoMatch)
86-
)
87-
}
79+
MatcherResult::Match => format!(
80+
"has character count, which {}",
81+
self.expected.describe(MatcherResult::Match)
82+
)
83+
.into(),
84+
MatcherResult::NoMatch => format!(
85+
"has character count, which {}",
86+
self.expected.describe(MatcherResult::NoMatch)
87+
)
88+
.into(),
8889
}
8990
}
9091

91-
fn explain_match(&self, actual: &T) -> String {
92+
fn explain_match(&self, actual: &T) -> Description {
9293
let actual_size = actual.as_ref().chars().count();
9394
format!(
9495
"which has character count {}, {}",
9596
actual_size,
9697
self.expected.explain_match(&actual_size)
9798
)
99+
.into()
98100
}
99101
}
100102

101103
#[cfg(test)]
102104
mod tests {
103105
use super::char_count;
106+
use crate::description::Description;
104107
use crate::matcher::{Matcher, MatcherResult};
105108
use crate::prelude::*;
106109
use indoc::indoc;
@@ -135,11 +138,11 @@ mod tests {
135138
false.into()
136139
}
137140

138-
fn describe(&self, _: MatcherResult) -> String {
141+
fn describe(&self, _: MatcherResult) -> Description {
139142
"called described".into()
140143
}
141144

142-
fn explain_match(&self, _: &T) -> String {
145+
fn explain_match(&self, _: &T) -> Description {
143146
"called explain_match".into()
144147
}
145148
}

0 commit comments

Comments
 (0)