Enforce consistent punctuation in algorithms (graphql#1069)

* Consistent punctuation in algorithms

* Clarify style guide

* Boolean formatting

* Add a format check to CI

Author: benjie

.github/algorithm-format-check.mjs

@@ -0,0 +1,114 @@
+import { readFile, readdir } from "node:fs/promises";
+
+const SPEC_DIR = new URL("../spec", import.meta.url).pathname;
+
+process.exitCode = 0;
+const filenames = await readdir(SPEC_DIR);
+for (const filename of filenames) {
+  if (!filename.endsWith(".md")) {
+    continue;
+  }
+  const markdown = await readFile(`${SPEC_DIR}/${filename}`, "utf8");
+
+  /**
+   * Not strictly 'lines' since we try and group indented things together as if
+   * they were one line. Close enough though.
+   */
+  const lines = markdown.split(/\n(?=[\S\n]|\s*(?:-|[0-9]+\.) )/);
+
+  for (let i = 0, l = lines.length; i < l; i++) {
+    const line = lines[i];
+
+    // Check algorithm is consistently formatted
+    {
+      // Is it an algorithm definition?
+      const matches = line.match(/^([a-z0-9A-Z]+)(\s*)\(([^)]*)\)(\s*):(\s*)$/);
+      if (matches) {
+        const [, algorithmName, ns1, _args, ns2, ns3] = matches;
+        if (ns1 || ns2 || ns3) {
+          console.log(
+            `Bad whitespace in definition of ${algorithmName} in '${filename}':`
+          );
+          console.log(line);
+          console.log();
+          process.exitCode = 1;
+        }
+        if (lines[i + 1] !== "") {
+          console.log(
+            `No empty space after algorithm ${algorithmName} header in '${filename}'`
+          );
+          console.log();
+          process.exitCode = 1;
+        }
+        for (let j = i + 2; j < l; j++) {
+          const step = lines[j];
+          if (!step.match(/^\s*(-|[0-9]+\.) /)) {
+            if (step !== "") {
+              console.log(
+                `Bad algorithm ${algorithmName} step in '${filename}':`
+              );
+              console.log(step);
+              console.log();
+              process.exitCode = 1;
+            }
+            break;
+          }
+          if (!step.match(/[.:]$/)) {
+            console.log(
+              `Bad formatting for '${algorithmName}' step (does not end in '.' or ':') in '${filename}':`
+            );
+            console.log(step);
+            console.log();
+            process.exitCode = 1;
+          }
+          if (step.match(/^\s*(-|[0-9]\.)\s+[a-z]/)) {
+            console.log(
+              `Bad formatting of '${algorithmName}' step (should start with a capital) in '${filename}':`
+            );
+            console.log(step);
+            console.log();
+            process.exitCode = 1;
+          }
+          const trimmedInnerLine = step.replace(/\s+/g, " ");
+          if (
+            trimmedInnerLine.match(
+              /(?:[rR]eturn|is (?:not )?)(true|false|null)\b/
+            ) &&
+            !trimmedInnerLine.match(/null or empty/)
+          ) {
+            console.log(
+              `Potential bad formatting of '${algorithmName}' step (true/false/null should be wrapped in curly braces, e.g. '{true}') in '${filename}':`
+            );
+            console.log(step);
+            console.log();
+            process.exitCode = 1;
+          }
+        }
+      }
+    }
+
+    // Check `- ...:` step is followed by an indent
+    {
+      const matches = line.match(/^(\s*)- .*:\s*$/);
+      if (matches) {
+        const indent = matches[1];
+        const nextLine = lines[i + 1];
+        if (!nextLine.startsWith(`${indent}  `)) {
+          console.log(
+            `Lacking indent in '${filename}' following ':' character:`
+          );
+          console.dir(line);
+          console.dir(nextLine);
+          console.log();
+          // TODO: process.exitCode = 1;
+        }
+      }
+    }
+  }
+}
+
+if (process.exitCode === 0) {
+  console.log(`Everything looks okay!`);
+} else {
+  console.log(`Please resolve the errors detailed above.`);
+}

.github/workflows/ci.yml

@@ -21,6 +21,7 @@ jobs:
       - uses: actions/setup-node@v3
       - run: npm ci
       - run: npm run test:format
+      - run: npm run test:algorithm-format
   test-build:
     runs-on: ubuntu-latest
     steps:

STYLE_GUIDE.md

@@ -56,7 +56,29 @@ All elements in hyphenated words follow the same rules, e.g. headings may
 contain `Non-Null`, `Context-Free`, `Built-in` (`in` is a preposition, so is not
 capitalized).
 
-## Lists
+## Algorithms
 
-Lists can be written as full sentences or as fragments. Algorithms that appear
-as lists, however, should be written in full sentences with proper punctuation.
+A named algorithm definition starts with the name of the algorithm in
+`PascalCase`, an open parenthesis, a comma-and-space separated list of
+arguments, a close parenthesis and then a colon. It is followed by a blank
+newline and a list of steps in the algorithm which may be numbered or bulleted.
+
+Each step in an algorithm should either end in a colon (`:`) with an indented
+step on the next line, or a fullstop (`.`). (A step after a step ending in a
+full stop may or may not be indented, use your discretion.)
+
+Indentation in algorithms is significant.
+
+Every step in an algorithm should start with a capital letter.
+
+```
+MyAlgorithm(argOne, argTwo):
+
+- Let {something} be {true}.
+- For each {arg} in {argOne}:
+  - If {arg} is greater than {argTwo}:
+    - Let {something} be {false}.
+  - Otherwise if {arg} is less than {argTwo}:
+    - Let {something} be {true}.
+- Return {something}.
+```

package.json

@@ -17,6 +17,7 @@
     "test:spelling": "cspell \"spec/**/*.md\" README.md",
     "format": "prettier --write \"**/*.{md,yml,yaml,json}\"",
     "test:format": "prettier --check \"**/*.{md,yml,yaml,json}\" || npm run suggest:format",
+    "test:algorithm-format": "node .github/algorithm-format-check.mjs",
     "suggest:format": "echo \"\nTo resolve this, run: $(tput bold)npm run format$(tput sgr0)\" && exit 1",
     "build": "./build.sh",
     "test:build": "spec-md --metadata spec/metadata.json spec/GraphQL.md > /dev/null",

spec/Section 2 -- Language.md

@@ -1032,7 +1032,7 @@ BlockStringValue(rawValue):
 - Let {lines} be the result of splitting {rawValue} by {LineTerminator}.
 - Let {commonIndent} be {null}.
 - For each {line} in {lines}:
-  - If {line} is the first item in {lines}, continue to the next line.
+  - If {line} is the first item in {lines}, continue to the next {line}.
   - Let {length} be the number of characters in {line}.
   - Let {indent} be the number of leading consecutive {WhiteSpace} characters in
     {line}.
@@ -1117,7 +1117,7 @@ ListValue : [ ]
 ListValue : [ Value+ ]
 
 - Let {inputList} be a new empty list value.
-- For each {Value+}
+- For each {Value+}:
   - Let {value} be the result of evaluating {Value}.
   - Append {value} to {inputList}.
 - Return {inputList}.
@@ -1164,7 +1164,7 @@ ObjectValue : { }
 ObjectValue : { ObjectField+ }
 
 - Let {inputObject} be a new input object value with no fields.
-- For each {field} in {ObjectField+}
+- For each {field} in {ObjectField+}:
   - Let {name} be {Name} in {field}.
   - Let {value} be the result of evaluating {Value} in {field}.
   - Add a field to {inputObject} of name {name} containing value {value}.
@@ -1247,22 +1247,22 @@ input type.
 
 Type : Name
 
-- Let {name} be the string value of {Name}
+- Let {name} be the string value of {Name}.
 - Let {type} be the type defined in the Schema named {name}
-- {type} must not be {null}
-- Return {type}
+- {type} must not be {null}.
+- Return {type}.
 
 Type : [ Type ]
 
-- Let {itemType} be the result of evaluating {Type}
+- Let {itemType} be the result of evaluating {Type}.
 - Let {type} be a List type where {itemType} is the contained type.
-- Return {type}
+- Return {type}.
 
 Type : Type !
 
-- Let {nullableType} be the result of evaluating {Type}
+- Let {nullableType} be the result of evaluating {Type}.
 - Let {type} be a Non-Null type where {nullableType} is the contained type.
-- Return {type}
+- Return {type}.
 
 ## Directives
 

spec/Section 3 -- Type System.md

@@ -347,22 +347,22 @@ can only be used as input types. Object, Interface, and Union types can only be
 used as output types. Lists and Non-Null types may be used as input types or
 output types depending on how the wrapped type may be used.
 
-IsInputType(type) :
+IsInputType(type):
 
 - If {type} is a List type or Non-Null type:
   - Let {unwrappedType} be the unwrapped type of {type}.
-  - Return IsInputType({unwrappedType})
+  - Return IsInputType({unwrappedType}).
 - If {type} is a Scalar, Enum, or Input Object type:
-  - Return {true}
+  - Return {true}.
 - Return {false}.
 
-IsOutputType(type) :
+IsOutputType(type):
 
 - If {type} is a List type or Non-Null type:
   - Let {unwrappedType} be the unwrapped type of {type}.
-  - Return IsOutputType({unwrappedType})
+  - Return IsOutputType({unwrappedType}).
 - If {type} is a Scalar, Object, Interface, Union, or Enum type:
-  - Return {true}
+  - Return {true}.
 - Return {false}.
 
 ### Type Extensions
@@ -919,7 +919,7 @@ of rules must be adhered to by every Object type in a GraphQL schema.
       3. The argument must accept a type where {IsInputType(argumentType)}
          returns {true}.
       4. If argument type is Non-Null and a default value is not defined:
-         - The `@deprecated` directive must not be applied to this argument.
+         1. The `@deprecated` directive must not be applied to this argument.
 3. An object type may declare that it implements one or more unique interfaces.
 4. An object type must be a super-set of all interfaces it implements:
    1. Let this object type be {objectType}.
@@ -1699,7 +1699,7 @@ input ExampleInputObject {
    3. The input field must accept a type where {IsInputType(inputFieldType)}
       returns {true}.
    4. If input field type is Non-Null and a default value is not defined:
-      - The `@deprecated` directive must not be applied to this input field.
+      1. The `@deprecated` directive must not be applied to this input field.
 3. If an Input Object references itself either directly or through referenced
    Input Objects, at least one of the fields in the chain of references must be
    either a nullable or a List type.

spec/Section 4 -- Introspection.md

@@ -414,8 +414,8 @@ The `__Field` type represents each field in an Object or Interface type.
 
 Fields\:
 
-- `name` must return a String
-- `description` may return a String or {null}
+- `name` must return a String.
+- `description` may return a String or {null}.
 - `args` returns a List of `__InputValue` representing the arguments this field
   accepts.
   - Accepts the argument `includeDeprecated` which defaults to {false}. If
@@ -433,8 +433,8 @@ The `__InputValue` type represents field and directive arguments as well as the
 
 Fields\:
 
-- `name` must return a String
-- `description` may return a String or {null}
+- `name` must return a String.
+- `description` may return a String or {null}.
 - `type` must return a `__Type` that represents the type this input value
   expects.
 - `defaultValue` may return a String encoding (using the GraphQL language) of
@@ -451,8 +451,8 @@ The `__EnumValue` type represents one of possible values of an enum.
 
 Fields\:
 
-- `name` must return a String
-- `description` may return a String or {null}
+- `name` must return a String.
+- `description` may return a String or {null}.
 - `isDeprecated` returns {true} if this enum value should no longer be used,
   otherwise {false}.
 - `deprecationReason` optionally provides a reason why this enum value is
@@ -489,8 +489,8 @@ supported. All possible locations are listed in the `__DirectiveLocation` enum:
 
 Fields\:
 
-- `name` must return a String
-- `description` may return a String or {null}
+- `name` must return a String.
+- `description` may return a String or {null}.
 - `locations` returns a List of `__DirectiveLocation` representing the valid
   locations this directive may be placed.
 - `args` returns a List of `__InputValue` representing the arguments this

spec/Section 5 -- Validation.md

@@ -435,25 +435,25 @@ SameResponseShape(fieldA, fieldB):
 - Let {typeA} be the return type of {fieldA}.
 - Let {typeB} be the return type of {fieldB}.
 - If {typeA} or {typeB} is Non-Null:
-  - If {typeA} or {typeB} is nullable, return false.
+  - If {typeA} or {typeB} is nullable, return {false}.
   - Let {typeA} be the nullable type of {typeA}.
   - Let {typeB} be the nullable type of {typeB}.
 - If {typeA} or {typeB} is List:
-  - If {typeA} or {typeB} is not List, return false.
+  - If {typeA} or {typeB} is not List, return {false}.
   - Let {typeA} be the item type of {typeA}.
   - Let {typeB} be the item type of {typeB}.
   - Repeat from step 3.
 - If {typeA} or {typeB} is Scalar or Enum:
-  - If {typeA} and {typeB} are the same type return true, otherwise return
-    false.
+  - If {typeA} and {typeB} are the same type return {true}, otherwise return
+    {false}.
 - Assert: {typeA} and {typeB} are both composite types.
 - Let {mergedSet} be the result of adding the selection set of {fieldA} and the
   selection set of {fieldB}.
 - Let {fieldsForName} be the set of selections with a given response name in
   {mergedSet} including visiting fragments and inline fragments.
 - Given each pair of members {subfieldA} and {subfieldB} in {fieldsForName}:
-  - If {SameResponseShape(subfieldA, subfieldB)} is false, return false.
-- Return true.
+  - If {SameResponseShape(subfieldA, subfieldB)} is {false}, return {false}.
+- Return {true}.
 
 **Explanatory Text**