Process feedback, progress #8

docs/chapters/add_player.md

@@ -34,8 +34,9 @@ The error will be that the `count_n_players` function is absent.
 The comment `Note 1` is to foreshadow
 that an `App` needs to be mutable to have its amount of players counted.
 It is unexpected that a read-only operation (i.e. counting the amount of players)
-requires the data it works on to be mutable. The Bevy library probably
-has good reasons why it must be mutable.
+requires the data it works on to be mutable.
+The Bevy library, however, has good reasons why `App` must be mutable:
+also reading data need to be done in a safe way.
 
 In general, a read-only operation should be able to work
 on an immutable data structure: when, for example, getting the name
@@ -74,7 +75,7 @@ Here is a possible implementation of `count_n_players`:
 ```rust
 fn count_n_players(app: &mut App) -> usize {
     let mut query = app.world_mut().query::<&Player>();
-    return query.iter(app.world()).len();
+    query.iter(app.world()).len()
 }
 ```
 

feedback/feedback_issue_8.md

@@ -1,44 +1,102 @@
 # https://github.com/richelbilderbeek/bevy_tdd_book/issues/8
 
-Feedback from @donedgardo below:
-
-I have had this draft for a while. The thoughts are unorganized, and haven’t had the chance to finish reading the book.
-
-But I thought early feedback is better than late.
-
-Introduction:
-
-I believe the focus of 100 percent coverage is not a good reason for TDD. Aiming for 100 percent is an ok goal yet it is unachievable (Eg You cant have test coverage on main.)
-
-A better reason for adopting TDD is to give developers the confidence that their changes work. As a byproduct of this newly found confidence, developers deliver value to customers at a high pace with minimal bugs. High quality product is the customers value for TDD; and high confidence, and easy of adding new features is the value to the developer.
-
-Pressing a button that tells you that its ok to deploy the new changes to your users confidently, this is what truly is all about.
-
-2.3.1
-The reason app has to be mutable is because of the update method.
-
-2.3.2
-Might benefit to remove the return and semicolon in the implementation to keep consistent the rustacian ways. Maybe you can take the opportunity to teach about the blue phase, refactoring, after making the test pass. I think it would be valuable to show new TDD devs what phase we are at either red, green or blue. Color coding each section to get that rhythm of fail, pass, clean up.
-
-2.3.4
-I dont like the fact that we just wrote a test that now will fail by modifying the create app to add a player. I would create a new test that create app doesn’t create a player, and another test where we test where we add the new add player system and test it does.
-
-An alternative is to initially test that it has a player from the start, and work toward making it pass, and refactor step to make it into its own system “add_player”. A concept I think this section is missing is that when you are going from red to green you write only enough code to make the test pass.
-
-When some is as experienced as us doing TDD we might skip these micro steps but for someone just learning I would suggest breaking those down even further.
-
-I feel that using create_app function to test individual systems will be hard to maintain our test. I suggest separating the app creation and systems under different test cases. The create app should just create the app, not very valuable tbh. The test to check if it crashes might be a better candidate for end to end test instead of an unit test. Thinking more about this I think first testing a system is more straightforward that testing the if the app crashes or not.
-
-2.4.1
-As I feared, create app is doing too much and by adding a feature of adding player sprite it forces us to change a lot of signatures of test that where using create_app.
-
-An idea would be to create a empty app on each test and add the system under test, and only those systems to keep test easier to maintain. Testing create app where it adds all systems will be hard to maintain, especially when systems that depend on third party plugins
-
-2.4.3
-I think it’s wasteful to add a test that just passes without any change requirements of the production code. Maybe im mistaken but the count_n_players should already be implemented. I guess what you mean is that the call signature if create app has to be modified here. 🤔 i would suggest rethinking this section. It seem lazy when you say I will not repeat myself here but maybe im missing something.
-
-2.4.5
-An important concept when doing TDD is to write only enough production (non-test code) to pass the test. In this section I think you are writing more than needed to pass the test. Eg the test don’t care about sprite or its transform yet in this section you are writing the code for it.
+> Feedback from @donedgardo below:
+> 
+> I have had this draft for a while. The thoughts are unorganized,
+> and haven’t had the chance to finish reading the book.
+> 
+> But I thought early feedback is better than late.
+> 
+> Introduction:
+> 
+> I believe the focus of 100 percent coverage is not a good reason for TDD.
+> Aiming for 100 percent is an ok goal yet it is unachievable
+> (Eg You cant have test coverage on main.)
+>
+> A better reason for adopting TDD is to give developers
+> the confidence that their changes work. As a byproduct of this newly
+> found confidence, developers deliver value to customers at a high pace with
+> minimal bugs. High quality product is the customers value for TDD;
+> and high confidence, and easy of adding new features is the value to the
+> developer.
+>
+> Pressing a button that tells you that its ok to deploy the new changes to your users confidently, this is what truly is all about.
+
+Edgardo has a point here: the goal of TDD is indeed not a high code coverage.
+I will consider after checking the literature.
+
+- [ ] TODO: check literature.
+
+
+> 2.3.1
+> The reason app has to be mutable is because of the update method.
+
+Changed to:
+
+'The Bevy library, however, has good reasons why `App` must be mutable: also reading data need to be done in a safe way.'
+
+I feel mentioning the 'update' method does not help the reader
+
+> 2.3.2
+> Might benefit to remove the return and semicolon in the implementation to
+> keep consistent the rustacian ways. Maybe you can take the opportunity to
+> teach about the blue phase, refactoring, after making the test pass.
+> I think it would be valuable to show new TDD devs what phase we are at either
+> red, green or blue. Color coding each section to get that rhythm of fail,
+> pass, clean up.
+
+I corrected the code: Eduardo is right. I wonder why clippy did not fix this, I guess
+because it is a test function ...
+
+Color-coding sections seems like a fun idea
+
+- [ ] Consider color-coding sections
+
+> 2.3.4
+> I dont like the fact that we just wrote a test that now will fail by
+> modifying the create app to add a player. I would create a new test that
+> create app doesn’t create a player, and another test where we test where we
+> add the new add player system and test it does.
+>
+> An alternative is to initially test that it has a player from the start,
+> and work toward making it pass, and refactor step to make it into its own
+> system “add_player”. A concept I think this section is missing is that when
+> you are going from red to green you write only enough code to make the test
+> pass.
+>
+> When some is as experienced as us doing TDD we might skip these micro steps
+> but for someone just learning I would suggest breaking those down even
+> further.
+
+> I feel that using create_app function to test individual systems will be
+> hard to maintain our test. I suggest separating the app creation and systems
+> under different test cases. The create app should just create the app,
+> not very valuable tbh. The test to check if it crashes might be a better
+> candidate for end to end test instead of an unit test. Thinking more about
+> this I think first testing a system is more straightforward that testing
+> the if the app crashes or not.
+
+> 2.4.1
+> As I feared, create app is doing too much and by adding a feature of adding
+> player sprite it forces us to change a lot of signatures of test that
+> where using create_app.
+
+> An idea would be to create a empty app on each test and add the system
+> under test, and only those systems to keep test easier to maintain.
+> Testing create app where it adds all systems will be hard to maintain,
+> especially when systems that depend on third party plugins
+
+> 2.4.3
+> I think it’s wasteful to add a test that just passes without any change
+> requirements of the production code. Maybe im mistaken but the
+> count_n_players should already be implemented. I guess what you mean is that
+> the call signature if create app has to be modified here. 
+> :thinking: i would suggest rethinking this section.
+> It seem lazy when you say I will not repeat myself here
+> but maybe im missing something.
+
+> 2.4.5
+> An important concept when doing TDD is to write only enough production (non-test code) to pass the test. In this section I think you are writing more than needed to pass the test. Eg the test don’t care about sprite or its transform yet in this section you are writing the code for it.
 
 I see now that you add the test for transorm afterwards which is ok when you are experienced, but for a new TDDer you need to break thibgs down in really small steps: