Re: [PATCH v2] Documentation: kunit: rewrite writing first test instructions

[Bagas Sanjaya <bagasdotme@gmail.com>]

On 9/30/22 13:42, David Gow wrote:
> 
> While I like the idea behind this, the wording probably needs a bit of
> tweaking. In addition, by describing everything in too much detail, I
> fear we might just be adding some needless redundancy. My suspicion is
> that everyone who's going to be writing KUnit tests already knows C
> (or has access to better learning materials than this), so we're
> unlikely to need to describe in detail that, e.g., misc_example_add()
> adds two numbers together when the code is right there,
> 

We should just say "First, write the driver implementation" (without
describing writing C code in detail), right?

>>
>> -.. code-block:: c
>> +   .. code-block:: c
> 
> Why are all of these code-block declarations being indented? It
> doesn't seem to affect the actual documentation build, so I guess it's
> harmless, but it'd be better not to have it change unnecessarily and
> clutter up the diff.
> 

The indentation for code-block directive is required, since the preceding
paragraph is multiline; otherwise there will be Sphinx warnings.

>>
>>         int misc_example_add(int left, int right);
>>
>> -2. Create a file ``drivers/misc/example.c``, which includes:
>> +   Then implement the function in ``drivers/misc/example.c``:
> 
>>
>> -.. code-block:: c
>> +   .. code-block:: c
> 
> Again, code-block indentation?

Yes, for consistency.

> 
>>
>>         #include <linux/errno.h>
>>
>> @@ -152,24 +154,25 @@ In your kernel repository, let's add some code that we can test.
>>                 return left + right;
>>         }
>>
>> -3. Add the following lines to ``drivers/misc/Kconfig``:
>> +2. Add Kconfig menu entry for the feature to ``drivers/misc/Kconfig``:
> 
> This needs rewording to add back an article ("a" or "the"), and we
> probably want to call this a "Kconfig entry" rather than a "Kconfig
> menu entry". Maybe "Add a Kconfig entry for the driver..."?
> 
>>
>> -.. code-block:: kconfig
>> +   .. code-block:: kconfig
> 
> Indentation again?

Yes, see above reply.

> 
>>
>>         config MISC_EXAMPLE
>>                 bool "My example"
>>
>> -4. Add the following lines to ``drivers/misc/Makefile``:
>> +3. Add the kbuild goal that will build the feature to
>> +   ``drivers/misc/Makefile``:
> 
> Kbuild goal? I've never heard of this being called a Kbuild goal before?
> 
> How about a "make target"?
> 

At the time of writing this patch, I use terminology in
Documentation/kbuild/makefiles.rst, which the "make target" is called
"Kbuild goal".

>>
>> -.. code-block:: make
>> +   .. code-block:: make
> 
> Indentation?

Yes, for consistency with the first code-block directive.

>>
>> -.. code-block:: c
>> +   .. code-block:: c
> 
> Indentation.
> 

See above reply.

>>
>> -.. code-block:: kconfig
>> +   .. code-block:: kconfig
> 
> Indentation?

See above reply.

>>
>> -.. code-block:: make
>> +   .. code-block:: make
> 
> Indentation?

See above reply.

>>
>> -.. code-block:: none
>> +   .. code-block:: none
> 
> Indentation?
> 

See above reply.

>>
>>         CONFIG_MISC_EXAMPLE=y
>>         CONFIG_MISC_EXAMPLE_TEST=y
>>
>>  5. Run the test:
>>
>> -.. code-block:: bash
>> +   .. code-block:: bash
> 
> Indentation?

See above reply.

Thanks for reviewing.

-- 
An old man doll... just what I always wanted! - Clara