Expand the doc comment for Context #1902

natebosch · 2023-02-03T05:01:27Z

This is the main point of interaction for authors of extensions, so docs
cover a wide range of information, including how the descriptions will
be used in failure output. Include some descriptions about how behavior
can vary across different contexts, and some high level descriptions
of the behavior for the contexts related to specific utilities.

The main benefit this brings is it brings more alignment with the `clause` arguments for `expect` calls. The docs will be able to focus on the difference in how the value is use (preceding "that" in the case of labels, standing on its own in a list in the case of clauses) and can use a consistent description for how it is passed. A secondary benefit is that it allows multiline labels and avoid workaround like joining with `r'\n'`. A final benefit is that it saves some unnecessary String formatting since the callback isn't called if no expectations fail on the Subject, or when used as a soft check where the failure details are ignored. - Make the `label` arguments to `nest` and `nestAsync`, and the _label field in `_TestContext` an `Iterable<String> Function()`. - Wrap strings that had been passed to `String` arguments with callbacks that return the string in a list. - When writing the label in a failure, write all lines, and use a postfix " that:". - Update some `Map` expectations which had manually joined with literal slash-n to keep the label or clause to a single line to take advantage of the multiline allowance. Split tests for the changed implementations and add tests for the descriptions with multiline examples. Some of these could have used multiline clauses before.

This is the main point of interaction for authors of extensions, so docs cover a wide range of information, including how the descriptions will be used in failure output.

lrhn

Lots of useful information, but I'd prefer some more conceptual stuff up-front, to set the stage for the technical details below. (Introduce concepts before referring to them, so that I know the what and why about them.)

lrhn · 2023-02-03T08:53:43Z

pkgs/checks/lib/src/checks.dart

 ///
-/// This is the surface of interaction for expectation extension method
-/// implementations.
+/// Expectation extension methods use [ContextExtension] to get access to the


If I start reading here (which I did), I don't know what an "expectation extension method" is. I assume it's an extension method.

Is there some overview documentation (probably elsewhere) which describes the general concepts and their relations, which can be linked to before going into details?

Is there some overview documentation (probably elsewhere) which describes the general concepts and their relations, which can be linked to before going into details?

Not currently. Where do you think the best place to put that would be? The README?

That, and/or the library documentation for the public libraries.

And for each important concept, describe itself thoroughly in its documentation, and then mention and link to the other relevant concepts, enough to show their relation.

lrhn · 2023-02-03T09:13:02Z

pkgs/checks/lib/src/checks.dart

 ///
-/// This is the surface of interaction for expectation extension method
-/// implementations.
+/// Expectation extension methods use [ContextExtension] to get access to the


This is a technical comment. It's too early for that in a comment describing such a fundamental concept.
Start out describing the role and purpose at a higher abstraction level before diving into technical details.
(Then I'll understand why the technical details are there and why I should care. 😁)

I'd order the following sections in order of introducing concepts before referencing them, and by most-useful/most-often-useful first.

Start by determining who the documentation is for. My guess is that a Context is a concept must important to people writing new expectation extension methods, not for people simply using the checks package to write tests. (That's why it's exported from a separate context.dart library.)
So, write towards those, people who need to understand how and when to use a Context.

The order of concepts, as I currently understand things, could be:

(What:) A context represents an abstract value to be tested, with enough information to

Describe how the value was obtained and

perform some test-related operations on the value.

There may or may not be an actual value, depending on, for example,
whether earlier operations succeeded or not.

(Why:) A [Context] is the underlying primitive used to write type-specific user-aimed expectation extension methods like the extension method [CoreChecks.equals] on [Subject].
The context provides an interface to the underlying test framework to report failures,
and generate readable descriptions for a failure.

(Relation to more known concept:) A [Subject] is a wrapper around a Context which hides the Context methods,
so that a [Subject] can freely have extension members of any name.
The [ContextExtension] extension allows accessing the [Context] of a [Subject]
when writing your own expectation extension methods.

(How to use:) There are two kinds of operations on a value that are exposed by a context:

Extracting new values from the current value.

Expecting something of the current value.

Both kinds of operation will be recorded and performed if/when an actual value is available.

Both kinds of operations also provide description callbacks that can be used
to give a textual representation of what is happening.

(And if no value can be supplied, it's possible to react to that too?)

Extracting a new value from the current value produces new context for the new value.

Use [nest] or [nestAsync] to describe how to extract a value.

The new context has the current context as "parent" context,
and the chain of contexts describe the operations used to get
to the value.

Has a label description callback to describe how the value is extracted.
It is called only when necessary.

Expecting something of a value records the expectation.

Use [expect], [expectAsync] or [expectUnawaited] methods to add an expectation.

Each expectation provides a predicate which can perform a test on an
actual value, and either report a [Rejection] or not, in a way which depends
on which method is used.

Has a clause description callback to create a textual description of the test being performed. It's called only when necessary.

<description of clause here>

Put "Where possible, string formatting for the descriptions should be performed in the callback, not ahead of time." here

(Behavior:) The actual behavior when a value is available is:

Extraction operations are performed, and the resulting value is provided
to the corresponding nested context.

(If that throws, it's considered a test failure?)

Expectations' predicates are tested against the value.

If they result in a rejection, the expectations fails.

<Section on expectation failing>

Otherwise, if all expectations succeed, <section on all expectations succeeding>.

I think most of the behavior-information is here, a lot of the rest too, but the order doesn't let me understand underlying concepts before I see technical details about them.

This is super helpful, and I agree it would be more clear to reorder the information.

Ok, I didn't use the exact order you suggested, but I used this structure for inspiration and I do think the doc is easier to read through now.

WDYT?

pkgs/checks/lib/src/checks.dart

lrhn · 2023-02-03T10:25:30Z

pkgs/checks/lib/src/checks.dart

+/// used and the callbacks will not be called. Where possible, string formatting
+/// for the descriptions should be performed in the callback, not ahead of time.
+///
+/// When an expectation is used on a [ConditionSubject] and passed to


What is a [ConditionSubject] and is it important here?
(Aka, what is its relation to Context?)

Should this just be documented on the ConditionSubject class itself? (Probably.)

I rephrased this so it's hopefully more clear.

lrhn · 2023-02-03T10:48:54Z

pkgs/checks/lib/src/checks.dart

-/// implementations.
+/// Expectation extension methods use [ContextExtension] to get access to the
+/// subject's [Context]. Expectations pass callbacks which describe what is
+/// checked, and perform the expectation, respectively.


"Expectations pass callbacks" ...

I read "expectations" as the abstract concept, not concrete code.

If so, expectations do not "pass callbacks". Checking an expectation might, because checking an expectation is a runtime operation, which can have the runtime behavior of passing callbacks.

(Also, needs defining the concept of "description callback".)

In general, be very careful about the use of the word "expectations".
It's generally used as an abstract conceptual thing, but it's easy to mix it with "expectation extension methods" (code concept) and the internal representation of an expectation.

I think I fixed it so that I consistently use "expectation extension method" (or in a few places "expectation extension"), and the term "expectation" should only be used in the more general "a thing that can be expected of a subject" sense.

In the user facing docs describing writing tests, I suspect it will feel natural to describe calling an expectation extension method on a subject as "checking an expectation" on the subject, or for a ConditionSubject "recording and expectation".

lrhn · 2023-02-03T10:52:15Z

pkgs/checks/lib/src/checks.dart

+/// description callbacks are used.
+///
+/// When both callbacks are used, the description callback will always be called
+/// strictly after the expectation callback is called.


Is this important? (That is: Do you have, or expect, code which depends on that order? If not, it might be safer to not make the promise. Such code would likely maintain internal state which makes it impossible to replay the same check/description on a separate value, which would make me very disappointed. I still want to do myIntegers.check.each > 0;, which means replaying the expectation, likely using a ConditionalSubject, on every element of the iterable.)

Do you have, or expect, code which depends on that order?

Yes. Sometimes where I can't do async work for the description callback, I might have a fallback description by default, but gather some async information during the predicate and format it for a richer description when possible.

For example the inOrder extension method cannot synchronously describe the Condition instances it will check because they will use async expectations. It has a fallback for describe calling out the number of conditions that need to be satisfied, but only shows their details when the actual has run.

test/pkgs/checks/lib/src/extensions/async.dart

Lines 221 to 224 in 911659b

await _expectAsync(

() => descriptions.isEmpty

? ['satisfies ${conditions.length} conditions in order']

: descriptions, (actual) async {

Such code would likely maintain internal state which makes it impossible to replay the same check/description on a separate value, which would make me very disappointed.

That is the case. We should investigate whether there is a different way to provide a good UX for this.
Maybe we can devise an explicit way for the expectation callback to communicate to the description callback that would be compatible with reuse. Or maybe we should sacrifice the UX for those use cases.

pkgs/checks/lib/src/checks.dart

Add a doc on the `_label` member. Add doc on `_child` constructor.

Expand description of label a bit. Remove reference to "single line" A future PR will make the line-per-element behavior more clear at the class level.

Start with more general information, and then get more specific. Add some dartdoc templates for bits that are useful to repeat in the docs for individual methods.

…e other docs

They don't need to be read externally, the callers only care about the two constructors, and these details are good to hide because the two nullable field pattern is ugly. This also fixes dartdoc references to the constructors with [Extracted.rejection]. Otherwise it would link to the field.

jakemac53

In general I think my main comment would be that there is no example code in these docs, or links to examples.

I think a lot of people learn best by example, and just linking to existing (built-in) extensions from several of the sections, or providing really basic examples inline, would help a lot.

pkgs/checks/lib/src/checks.dart

jakemac53 · 2023-02-06T18:12:13Z

pkgs/checks/lib/src/checks.dart

+///
+///
+/// Whichever type of operation, an expectation extension method provides two
+/// callbacks.


I think some sort of link to the api here would be good, to show how these are provided, it's a bit confusing without that imo. I end up wanting to interrupt by reading to find that.

I expanded this to show one callback signature, and describe that the other signature varies. Does that help?

What I am confused by here isn't what the callbacks are supposed to do, its how/where they are provided.

I think this stems from the mentions of expect etc being below this - these are describing how those work right? Feels like its putting the cart before the horse a bit, I don't have the context yet for what is being described.

How about mentioning each method in the bullet list above of the two types of operations?

natebosch · 2023-02-06T18:49:15Z

or providing really basic examples inline

I was a little worried about the docs getting too long. I also don't know how much we want to repeat between the README and the API docs - maybe we should be OK with copy pasting a bunch between them?

Would it work to add an example usage similar to what is in the README on each of the individual APIs, and leave the class level doc as is?

jakemac53 · 2023-02-06T19:10:09Z

I was a little worried about the docs getting too long. I also don't know how much we want to repeat between the README and the API docs - maybe we should be OK with copy pasting a bunch between them?

Would it work to add an example usage similar to what is in the README on each of the individual APIs, and leave the class level doc as is?

I think copy/pasting between them is fine. I do think the class level doc here should have examples, or link to examples.

natebosch · 2023-02-06T19:54:19Z

I do think the class level doc here should have examples, or link to examples.

I added a couple sync examples, one of expect and one of nest. It is towards the end of the doc. I added examples of all operations to the respective docs.

I also moved the detail about when callbacks when/won't be called towards the end of the doc. It's probably a detail most folks won't care about - it motivates the design but isn't important to know to use the design. It also now follows another mention of softCheck which I think makes it less confusing.

Had done a tweak in `has` that I noticed when copy/pasting it as an example, but then I decided to go with creating fake examples for all the operations because we don't have any nice small example for `expectAsync`.

natebosch added 3 commits February 3, 2023 02:39

Expand the doc comment for Context

7a0724f

This is the main point of interaction for authors of extensions, so docs cover a wide range of information, including how the descriptions will be used in failure output.

Compare the non-nesting label case to a clause directly

10e6f47

natebosch mentioned this pull request Feb 3, 2023

Make label arguments take callbacks #1900

Merged

Mention that which can be omitted

bac2b3e

natebosch marked this pull request as ready for review February 3, 2023 05:38

natebosch requested review from jakemac53 and lrhn February 3, 2023 05:38

lrhn approved these changes Feb 3, 2023

View reviewed changes

natebosch added 4 commits February 4, 2023 01:25

Enforce single string still in root ctor

4e4cf10

Add a doc on the `_label` member. Add doc on `_child` constructor.

Rename _StringClause to _ExpectationClause

00888e6

Update docs for nest and nestAsync

308b13d

Expand description of label a bit. Remove reference to "single line" A future PR will make the line-per-element behavior more clear at the class level.

Consistently use double quotes to avoid escape single quote

e35574b

Base automatically changed from label-callback to master February 4, 2023 01:56

natebosch added 6 commits February 4, 2023 02:05

Merge commit 'e35574bb' into label-callback--context-docs

915652d

Merge branch 'master' into label-callback--context-docs

511fa70

Attempt to present information in a better order

911659b

Start with more general information, and then get more specific. Add some dartdoc templates for bits that are useful to repeat in the docs for individual methods.

More wording tweaks, add doc header on ConditionSubject

547abd3

More wording tweaks and a paragraph on softCheck subjects

ef6bd7e

Move the 'should not throw' phrase into a template that is used in th…

99f9e2f

…e other docs

natebosch requested a review from lrhn February 4, 2023 06:32

natebosch added 5 commits February 4, 2023 19:52

More working tweaks, export ConditionSubject

db4fc6f

Merge branch 'master' into label-callback--context-docs

b33a327

Expectation extension method in Subject doc

330c3f6

missing word

4d14182

jakemac53 reviewed Feb 6, 2023

View reviewed changes

Link to example extensions, expand callback phrasing

6a8e670

natebosch requested a review from jakemac53 February 6, 2023 18:46

Link the specific methods earlier

afc09b3

natebosch added 2 commits February 6, 2023 19:13

Move section on callbacks being unused to much later

148ef9d

Add some examples

bae48cb

natebosch added 2 commits February 6, 2023 19:57

Revert mistaken change

e09f2ae

Had done a tweak in `has` that I noticed when copy/pasting it as an example, but then I decided to go with creating fake examples for all the operations because we don't have any nice small example for `expectAsync`.

Merge branch 'master' into label-callback--context-docs

b9b8960

jakemac53 approved these changes Feb 6, 2023

View reviewed changes

natebosch added 2 commits February 6, 2023 21:35

typo

830bdd4

Merge branch 'master' into label-callback--context-docs

fca6584

natebosch merged commit f2d97bf into master Feb 6, 2023

natebosch deleted the label-callback--context-docs branch February 6, 2023 22:02

	await _expectAsync(
	() => descriptions.isEmpty
	? ['satisfies ${conditions.length} conditions in order']
	: descriptions, (actual) async {

Expand the doc comment for Context #1902

Expand the doc comment for Context #1902

Uh oh!

Conversation

natebosch commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrhn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jakemac53 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natebosch commented Feb 6, 2023

Uh oh!

jakemac53 commented Feb 6, 2023

Uh oh!

natebosch commented Feb 6, 2023

Uh oh!

Uh oh!

natebosch commented Feb 3, 2023 •

edited

Loading