Review of simple form chapter

chapter_15_simple_form.asciidoc

@@ -119,7 +119,8 @@ class ItemFormTest(TestCase):
         self.assertIn('class="form-control form-control-lg"', form.as_p())
 ----
 ====
-
+// DAVID: what about splitting this up into arrange/act/assert and calling .as_p() only once
+// (in the act block?)
 
 
 That gives us a fail which justifies some real coding.
@@ -167,6 +168,7 @@ And then:
         ),
 ----
 ====
+// DAVID: the tests still fail. Should the second class here be form-control-lg, as in the test?
 
 NOTE: Doing this sort of widget customisation would get tedious
     if we had a much larger, more complex form.
@@ -194,6 +196,9 @@ and rewrite it properly later).
 
 Here we're actually using a unit test as a way of experimenting with the forms API.
 It's actually a pretty good way of learning how it works.
+
+// DAVID: double "actually".
+
 *******************************************************************************
 
 // SEBASTIAN: Small suggestion - I'd appreciate mentioning breakpoint() for use in test
@@ -208,6 +213,8 @@ We want our form to reuse the validation code that we've already defined on our
 Django provides a special class that can autogenerate a form for a model, called `ModelForm`.
 As you'll see, it's configured using a special attribute called `Meta`:
 
+// DAVID: would it be more accurate to say "using a special inner class called `Meta`"?
+
 [role="sourcecode"]
 .src/lists/forms.py
 ====
@@ -397,6 +404,11 @@ from lists.forms import EMPTY_ITEM_ERROR, ItemForm
 
 And the tests still pass:
 
+// DAVID: Might be better to update the test to use the constant at this point,
+// too. It's a bit distracting when it happens later in the chapter.
+// (Might be worth mentioning that it's good for tests not to require changing if
+// you change a piece of configuration like this EMPTY_ITEM_ERROR constant.)
+
 ----
 OK
 ----
@@ -605,6 +617,8 @@ We need to find any occurrences of the old `id` (`id_new_item`)
 and `name` (`item_text`) and replace them too,
 with `id_text` and `text`, respectively:
 
+// DAVID: this instruction could be given more clearly.
+
 [role="ignore-errors"]
 [subs="specialcharacters,quotes"]
 ----
@@ -1102,6 +1116,7 @@ class ItemValidationTest(FunctionalTest):
         self.wait_for_row_in_list_table("2: Make tea")
 ----
 ====
+// DAVID: Is it just me or do the earlier chapters have it as 'Buy milk'?
 
 <1> Instead of checking for our custom error message,
     we check using the CSS pseudoselector `:invalid`,
@@ -1139,7 +1154,7 @@ doing a refactor that's quite involved...this
 is the kind of thing that, without tests, would seriously worry me.
 In fact, I might well have decided
 that it wasn't worth messing with code that works.
-But, because we have a full tests suite, we can delve around,
+But, because we have a full test suite, we can delve around,
 tidying things up, safe in the knowledge
 that the tests are there to spot any mistakes we make.
 It just makes it that much likelier that you're going to keep refactoring,
@@ -1183,6 +1198,16 @@ so you always need the server side as well
 if you really care about data integrity;
 using a form is a nice way of encapsulating that logic.
 
+// DAVID: I wonder if this is slightly missing the point.
+// Our functional tests here are very high level - using a headless browser.
+// But many developers use the Django test client
+// (https://docs.djangoproject.com/en/5.1/topics/testing/tools/#the-test-client)
+// for this kind of thing, which doesn't have browser validation built in.
+// So we could use that if we really wanted to test the server side validation.
+//
+// This might be a good opportunity to mention that as an option
+// (even if we don't go into detail here)
+// and discuss the trade offs of using Selenium versus the test client, versus both.
 
 ((("HTML5")))
 Also, not all browsers ('cough--Safari--cough')
@@ -1204,8 +1229,6 @@ None of us can see the future,
 and we should concentrate on finding the right solution
 rather than the time "wasted" on the wrong solution.
 
-
-
 ==== Using the Form's Own Save Method
 
 
@@ -1283,6 +1306,9 @@ Here's how we can implement a custom save method:
 ----
 ====
 
+// DAVID: worth showing where this goes in the class, in case someone is thrown by the weird inner class?
+// (I know it doesn't actually matter, but might reduce friction.)
+
 The `.instance` attribute on a form represents the database object
 that is being modified or created.
 And I only learned that as I was writing this chapter!
@@ -1298,6 +1324,7 @@ Ran 23 tests in 0.086s
 OK
 ----
 
+// DAVID: Maybe I took a wrong turn somewhere but I only have 22 tests.
 
 Finally, we can refactor our views. `new_list` first:
 
@@ -1408,3 +1435,8 @@ Each test should test one thing::
     ((("unit tests", "testing only one thing")))
     ((("testing best practices")))
 *******************************************************************************
+
+// DAVID: Is that `+` a typo? It renders as-is in the html version of the page when I build it locally.
+
+// DAVID: Second tip: should this specify unit tests? It's certainly not the case for our functional tests.
+// Also this tip is missing the 'why', i.e. that it makes it easier to identify the precise bug.