Skip to content

Docs: StyleGuide.md as single source of truth for coding style - #101

Open
omonien wants to merge 2 commits into
MHumm:developmentfrom
omonien:docs/style-guide
Open

Docs: StyleGuide.md as single source of truth for coding style#101
omonien wants to merge 2 commits into
MHumm:developmentfrom
omonien:docs/style-guide

Conversation

@omonien

@omonien omonien commented Jul 23, 2026

Copy link
Copy Markdown

Summary

  • Adds Docs/StyleGuide.md as the single source of truth for DEC coding style, structure, and contribution expectations for new / rewritten code.
  • Consolidates former scattered notes from CONTRIBUTING.md, DEC65.pdf §3.7.1 (structure & style), de-facto library conventions (DEC… units, Apache headers, FPC/Delphi uses), and adapted practices from the modern Delphi Style Guide (naming prefixes, formatting, XML docs).
  • Reworks CONTRIBUTING.md into a short process entry point that points at the style guide.
  • Updates readme.md so contributors find the style guide next to the other project docs.

Policy (explicit)

  • No production / existing source reformatting in this PR (docs only).
  • New code and substantial rewrites (including future feature PRs such as ChaCha re-land) should be reviewed against Docs/StyleGuide.md.
  • Algorithm how-to (DEC65.pdf §3.7.2+) stays in the PDF; when the PDF is next regenerated, §3.7.1 can shrink to a pointer at Docs/StyleGuide.md.

Test plan

  • Docs-only review: read Docs/StyleGuide.md for DEC fit (FPC, unit naming, headers).
  • Confirm CONTRIBUTING.md and readme.md links resolve.
  • No .pas / build changes — nothing to compile.

Introduce a compact DEC style guide consolidating CONTRIBUTING rules,
DEC65.pdf section 3.7.1 structure notes, de-facto library conventions,
and adapted Delphi Style Guide practices. Point CONTRIBUTING and readme
at the guide; enforce for new/rewritten code only, without reformatting
existing sources.
@omonien
omonien force-pushed the docs/style-guide branch from af6cff9 to f10a90b Compare July 27, 2026 09:17
@MHumm

MHumm commented Sep 7, 2026

Copy link
Copy Markdown
Owner

I'm currently reading the styleguide, as far as I am it's mostly good. Eight things:

  1. What does re-lands in "donor re-lands such" mean?

  2. This example
    '''procedure TExample.DoWork(const AValue: string);
    begin
    if AValue = '' then
    raise EDECException.Create('Value required');

if IsReady then
begin
Process(AValue);
end;
end;'''

Is not too clever. In this example I wouldn't wrap Process(AValue); in begin / end, as this is simply unnecessary.
We should put at least another command in this begin / end block and describe that such a single call shouldn't get wrapped as that's simply superflous.

  1. I would ammend this one like shown here: Non-portable code | Wrap in {$IFDEF} so FPC (or unsupported platforms) do not see it. Use DECOptions.inc as the central place for turning on/off specific defines you added.
    So it's clear where such defines like classregistration etc. should be turned on/off/"defined".

  2. I wouldn't write "avoid withs". I would simply forbid it ;-) We're not C-programmers trying to save each keypress ;-)

  3. The list of Type and Interface prefixes misses simple type, e.g. if somebody declares TIndex = Integer the T prefix should be used as well, not only for records.

  4. If we mention this: Record public fields have no F prefix (they are part of the value layout). we would mention this for other public fields as well. Where's the value of this sentence? No Public field should have a F prefix. If the word itsself starts with F this is kept of course. Example: property FileName: string...

  5. Constants: Where was this one deduced from? String constant | sc | scInvalidNonce
    There are some string constants starting with sC, but only because the 1st word of the constant is Cipher.
    sCipherSomething... So I'd change this to string constants start with s.

  6. The example in Documentation comments does not describe AInitVector. For some it might be an obvious parameter for others it might not be (e.g. folks new to cryptography) so it would be better to have a rule to document all parameters.

@omonien

omonien commented Sep 8, 2026

Copy link
Copy Markdown
Author

Hallo Markus,

danke fürs sorgfältige Lesen. Kurz zu allen acht Punkten — alles StyleGuide für neuen Code, kein Massen-Rewrite der bestehenden Units.

  1. „re-lands“ war unglückliches Englisch. Gemeint: Code, der über einen Pull Request kommt („donor“), muss dem StyleGuide folgen. Beispiel ChaCha20/Poly1305 aus Basically a first implementation of the chacha20 Poly1305 algorithm #90: Wenn wir das später übernehmen, räumen wir Naming/Struktur/Docs nach dem Guide auf, statt den Donor-Stil 1:1 zu kopieren. Formuliere ich im Guide schlichter.

  2. Das begin/end um eine einzelne Anweisung ist Absicht. Bei Code, den mehrere Leute pflegen, zählt Eindeutigkeit mehr als Kürze — auch wenn es länger aussieht. Klassiker: zweite Zeile eingefügt, Einrückung wirkt richtig, gebunden ist nur die erste. Ausnahme bleiben nacktes Exit/raise/Continue/Break. Beispiel und Satz im Guide schärfe ich so, dass die Absicht klar wird.

  3. Einverstanden: eigene Defines zentral in DECOptions.inc — nehme ich in die Non-portable-Zeile auf.

  4. Einverstanden: with wird verboten, nicht nur „avoid“.

  5. T bei Typaliasen machen wir zur Empfehlung, kein Muss. Oft sinnvoll (TIndex = Integer), manchmal aber unpraktisch (RTL-artige Aliase, z. B. String-Namen ohne T).

  6. Hier gehe ich etwas weiter als die reine Namensfrage: Öffentliche Klassenfelder sollten vermieden werden (Properties statt Felder). Kein hartes Verbot wegen der historischen Codebasis, und keine Namensregel für öffentliche Felder — sonst legalisieren wir sie durch die Hintertür. Gründe: sie wirken wie Globals (State ohne klare Get/Set-Zuständigkeit), Feld vs. Property ist im Code unklar, keine Schreib-/Leserechte. Viele öffentliche Felder deuten oft eher auf ein Record (oder Architekturbruch) hin. Bei Records bleiben öffentliche Felder normal (Wertlayout).

  7. Einverstanden: String-Konstanten einheitlich mit s…, wie in DEC üblich. sc war missverständlich (sCipher… ist „s“ + Cipher, kein Prefix sc).

  8. Einverstanden: Bei neuer public API alle Parameter dokumentieren. Beispiel im Guide ergänze ich um AInitVector.

Wenn das für dich passt, arbeite ich das im StyleGuide nach.

Viele Grüße
Olaf

Clarify donor/PR code, keep begin/end for single statements, point
defines at DECOptions.inc, ban with, recommend T for type aliases,
avoid public class fields, use s for string constants, and require
XML <param> for every public API parameter.

Co-authored-by: Olaf Monien <omonien@users.noreply.github.com>
@MHumm

MHumm commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Ja bitte nacharbeiten. Geht in die richtige Richtung.
Mir kam noch ein Ergänzungsvorschlag:
Den Einbau von Asserts für alle Objektreferenz Parameter fordern die nicht als optional definiert sind.
Bei normalen Parametern würde ich das nicht hart fordern, da manchmal nicht ganz klar ist was erlaubt und was außer dem Wertebereich ist, v.a. bei Routinen die allgemeinerer Natur sind.

Vielleicht so was in Kapitel 5.2 Variables and parameters:

Asserts
Non optional object reference or pointer typed parameters must be guarded with Assert(Assigned(ParamName), 'ParamName is not assigned');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants