Skip to content

Commit 43aa5ae

Browse files
committed
[TASK] Enhance "reviewing guides" and note Xliff specialities
See: https://forge.typo3.org/issues/107008 https://typo3.slack.com/archives/CR75200FL/p1751359303847539
1 parent f1376a0 commit 43aa5ae

File tree

2 files changed

+32
-21
lines changed

2 files changed

+32
-21
lines changed

Documentation/Appendix/Cgl.rst

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,5 @@ Xliff files
5050
Language files are usually stored in a Folder Resources/Private/Language
5151
in files with the ending *.xlf*. While no tabs are allowed to indent
5252
in PHP files, you should edit Xliff files using tabs.
53-
53+
Please also check :ref:`common-review-checks-xlf` for Xliff-specific things
54+
to pay attention to.

Documentation/HandlingAPatch/Tips.rst

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -276,46 +276,56 @@ Consider the :ref:`policies and tips for voting patches <gerrit-voting>`.
276276

277277
.. _common-review-checks:
278278

279-
Common code review checks
280-
-------------------------
279+
Common code review checks / checklist
280+
-------------------------------------
281281

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-
* Is the code-flow readable? Does it need more (or less) comments? Any code complexities
285+
* Is the **code-flow readable**? Does it need more (or less) comments? Any **code complexities**
286286
("cognitive complexity") that could be easier to read when using different conditions/loops/sub-methods?
287287
* Do breaking changes occur that need to be noticed? This can also apply to:
288288

289-
* Type Hinting
289+
* **Type hinting / type declarations**
290290
* using PHP features beyond the supported PHP version
291291
* Loss of existing functionality
292292
* Typos
293293

294-
* Are new class, method, function, variable names understandable
295-
* Is there a need to add unit/functional testing for specific changes
296-
* Are regression tests for a bugfix needed?
297-
* Is the "Releases: " scope of a patch spanning the proper TYPO3 versions
294+
* Are new class, method, function, **variable names understandable**
295+
* Is there a need to add **unit/functional testing** for specific changes
296+
* Are **regression tests** for a bugfix needed?
297+
* Is the **"Releases: " scope** of a patch spanning the proper TYPO3 versions
298298
(depending on the state of current LTS and priority-bugfix-only releases)
299-
* Are the "final" and "private/protected" and "readonly" scopes of classes,
299+
* Are the **"final" and "private/protected" and "readonly" scopes** of classes,
300300
methods and variables used properly?
301-
* Is Dependency Injection used where applicable?
302-
* Are possibilities for early code returns and reduced nesting levels addressed?
301+
* Is **Dependency Injection** used where applicable?
302+
* Are possibilities for **early code returns** and reduced **nesting levels** addressed?
303303
* If a change is Breaking or comes with a larger impact: Is
304-
a "Breaking.rst" or "Important.rst" document part of the patch?
305-
* If a patch is a new feature: Is a "Feature"-rst part of the patch? Check
304+
a **"Breaking.rst" or "Important.rst"** document part of the patch?
305+
* If a patch is a new feature: Is a **"Feature"-rst** part of the patch? Check
306306
out the `rst file generator <https://forger.typo3.com/utilities/rst>`
307307
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?
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?
310310
* 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?
311+
* When **new PHP files** are added, do they contain the TYPO3 License and a `declare strict_types` header?
312312
* Is the provided commit message, reST files and the code itself aligned? Sometimes in the process
313313
of reworking a patch multiple times, these three place of documentation can become out of sync.
314-
* If SCSS/TS changes are involved, are the resulting build files included in
314+
* If **SCSS/TypeScript** changes are involved, are the resulting build files included in
315315
the patch, and were built with the right environment?
316-
* When changes are made to Xliff files, is the correct spelling in American (US) English used?
317-
* Does your patch deprecate anything? If so, have you followed :ref:`deprecations`?
316+
* 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**.
327+
318328

319329
If you feel this section is missing good things to watch out for for, please
320330
contribute to the documentation. This list does not claim to be complete, but
321-
should act as a kind of "Cheat-Sheet" for reviewing.
331+
should act as a kind of "Cheat-Sheet" or Checklist for reviewing.

0 commit comments

Comments
 (0)