Skip to content

Commit e9953a4

Browse files
committed
[TASK] Add subsections
1 parent f972735 commit e9953a4

File tree

1 file changed

+49
-18
lines changed

1 file changed

+49
-18
lines changed

Documentation/HandlingAPatch/Tips.rst

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,10 @@ Common code review checks / checklist
282282
For reviewing and giving feedback, here's a couple of things that are often addressed. You can
283283
use this list both for checking other people's patches, as well as your own.
284284

285+
.. _common-review-checks-general:
286+
General checks
287+
~~~~~~~~~~~~~~
288+
285289
* Is the **code-flow readable**? Does it need more (or less) comments? Any **code complexities**
286290
("cognitive complexity") that could be easier to read when using different conditions/loops/sub-methods?
287291
* Do breaking changes occur that need to be noticed? This can also apply to:
@@ -292,39 +296,66 @@ use this list both for checking other people's patches, as well as your own.
292296
* Typos
293297

294298
* Are new class, method, function, **variable names understandable**
299+
* Are possibilities for **early code returns** and reduced **nesting levels** addressed?
300+
* If **SCSS/TypeScript** changes are involved, are the resulting build files included in
301+
the patch, and were built with the right environment?
302+
303+
.. _common-review-checks-testing:
304+
Testing
305+
~~~~~~~
306+
295307
* Is there a need to add **unit/functional testing** for specific changes
296308
* Are **regression tests** for a bugfix needed?
309+
310+
.. _common-review-checks-formalities:
311+
Formalities
312+
~~~~~~~~~~~
313+
297314
* Is the **"Releases: " scope** of a patch spanning the proper TYPO3 versions
298315
(depending on the state of current LTS and priority-bugfix-only releases)
316+
* Does the **licensing** of any foreign code introduced match the Core licensing?
317+
* Is the **commit message complete**, clear and properly mentions all part of a patch?
318+
* If a new Exception is added, is the timestamp-identifier unique and recent?
319+
* When **new PHP files** are added, do they contain the TYPO3 License and a `declare strict_types` header?
320+
321+
.. _common-review-checks-php:
322+
PHP specifics
323+
~~~~~~~~~~~~~
324+
299325
* Are the **"final" and "private/protected" and "readonly" scopes** of classes,
300326
methods and variables used properly?
301327
* Is **Dependency Injection** used where applicable?
302-
* Are possibilities for **early code returns** and reduced **nesting levels** addressed?
328+
329+
.. _common-review-checks-docs:
330+
Documentation
331+
~~~~~~~~~~~~~
332+
303333
* If a change is Breaking or comes with a larger impact: Is
304334
a **"Breaking.rst" or "Important.rst"** document part of the patch?
305335
* If a patch is a new feature: Is a **"Feature"-rst** part of the patch? Check
306-
out the `rst file generator <https://forger.typo3.com/utilities/rst>`
336+
out the `rst file generator <https://forger.typo3.com/utilities/rst>`_
307337
for help on creating files like this.
308-
* Does the **licensing** of any foreign code introduced match the Core licensing?
309-
* Is the **commit message complete**, clear and properly mentions all part of a patch?
310-
* If a new Exception is added, is the timestamp-identifier unique and recent?
311-
* When **new PHP files** are added, do they contain the TYPO3 License and a `declare strict_types` header?
312338
* Is the provided commit message, reST files and the code itself aligned? Sometimes in the process
313339
of reworking a patch multiple times, these three place of documentation can become out of sync.
314-
* If **SCSS/TypeScript** changes are involved, are the resulting build files included in
315-
the patch, and were built with the right environment?
316340
* Does your patch **deprecate** anything? If so, have you followed :ref:`deprecations`?
317-
.. _common-review-checks-xlf:
318-
* When changes are made to **Xliff files** (translations):
319-
320-
* Is the **correct spelling** in American (US) English used?
321-
* Does the file use **proper indentation levels and characters** (tab)?
322-
* If an **existing** language key is changed on a LTS branch, it must **NOT introduce new
323-
argument placeholders** or remove existing ones. Instead, a new language key has to be
324-
introduced (and the old one can be deprecated/documented as outdated). This is
325-
important because localizations are delivered for any TYPO3 patch-level version,
326-
and could cause PHP exceptions when **argument placeholders mismatch**.
327341

342+
.. _common-review-checks-xlf:
343+
Xliff / language files
344+
~~~~~~~~~~~~~~~~~~~~~~
345+
346+
When changes are made to **Xliff files** (translations):
347+
348+
* Is the **correct spelling** in American (US) English used?
349+
* Does the file use **proper indentation levels and characters** (tab)?
350+
* If an **existing** language key is changed on a LTS branch, it must **NOT introduce new
351+
argument placeholders** or remove existing ones. Instead, a new language key has to be
352+
introduced (and the old one can be deprecated/documented as outdated). This is
353+
important because localizations are delivered for any TYPO3 patch-level version,
354+
and could cause PHP exceptions when **argument placeholders mismatch**.
355+
356+
.. _common-review-checks-missing:
357+
More?
358+
~~~~~
328359

329360
If you feel this section is missing good things to watch out for, please
330361
contribute to the documentation. This list does not claim to be complete, but

0 commit comments

Comments
 (0)