feat: Support multiple schema types #203

DafyddLlyr · 2024-07-16T10:38:14Z

What does this PR do?

Refactors repository to support multiple schemas instead of a single schema
- Type Schema is now DigitalPlanningApplication
Simplifies build scripts by using a shared bash script
Simplifies example generation to walk directories to generate JSON files instead of having to manually import each one
Tests: Walk /examples to import all JSON files, avoid manual imports

Changes to folder structure

The changes made are as follows -

/schema/schema.json → /schemas/${SCHEMA}.json
/examples/someExample.json → examples/${SCHEMA}/someExample.json
/types/Schema.ts → /types/${SCHEMA}.index.ts
/types/utils.ts → /types/shared/utils.ts
- I think more will end up in the /shared folder (maybe some enums?). The main motivation for moving utils there now (before it's actually shared) was just to be sure that these types would still be accessible to ts-json-schema-generator when they were "outside" / "above" the scope of the type being converted. Good news is that it works!

Tree view

schemas/
└── ${SCHEMA}/
    └── ${SCHEMA}.json

examples/
└── ${SCHEMA}/
    ├── data/
    │    └── example.ts
    └── example.json

types/
├── schemas/
│   └── ${SCHEMA}/
│       └── index.ts
└── shared/
    └── utils.ts

DafyddLlyr · 2024-07-16T14:48:50Z

.eslintrc.json

@@ -1,7 +1,7 @@
 {
  "extends": "./node_modules/gts/",
  "rules": {
-    "node/no-unpublished-import": ["error", {
+    "n/no-unpublished-import": ["error", {


Required by gts 5.3.1 - https://github.com/google/gts/releases/tag/v5.3.1

Bug Fixes

deps: replace dependency eslint-plugin-node with eslint-plugin-n (google/gts#865) (efbe3a8)

…r structure (#202)

… examples manually

DafyddLlyr · 2024-07-16T14:51:47Z

.github/workflows/publish.yml

@@ -38,7 +38,7 @@ jobs:
        run: |
          VERSION=v${{steps.version_check.outputs.version}} pnpm build
          mkdir -p "$GITHUB_WORKSPACE/v${{ steps.version_check.outputs.version }}"
-          mv schema/* "$GITHUB_WORKSPACE/v${{ steps.version_check.outputs.version }}"
+          mv schemas/* "$GITHUB_WORKSPACE/v${{ steps.version_check.outputs.version }}"


I think this is the only change needed here!

This has the nice benefit of preserving the old URLs in the format -

https://theopensystemslab.github.io/digital-planning-data-schemas/<VERSION>/schema.json

and generating new URLs in the format -

https://theopensystemslab.github.io/digital-planning-data-schemas/<VERSION>/schemas/<SCHEMA>.json

DafyddLlyr · 2024-07-16T14:56:24Z

examples/digitalPlanningApplication/data/landDrainageConsent.ts

@@ -1,9 +1,9 @@
-import {Schema} from '../../types/Schema';
-import {BaseProposal} from '../../types/schema/data/Proposal';
+import {DigitalPlanningApplication} from '../../../types/schemas/digitalPlanningApplication';


I'll have a look (in another PR) at path aliases in our tsconfig.json - it could be nice to import from @types, @utils etc now that these import paths are more deeply nested.

DafyddLlyr · 2024-07-16T14:59:05Z

scripts/build-schema.sh

+dirs=("digitalPlanningApplication")
+types=("DigitalPlanningApplication")


When we get new schemas, we'll just need to add them to this list, e.g.

dirs=("digitalPlanningApplication", "mySchemaDirectory") types=("DigitalPlanningApplication", "MySchemaType")

DafyddLlyr · 2024-07-16T21:26:06Z

types/schemas/digitalPlanningApplication/index.ts

+ * @title Digital Planning Application
+ * @description The root specification for a planning application in England generated by a digital planning service
+ */
+export interface DigitalPlanningApplication {


Something to consider - is DigitalPlanningApplication actually the right name? Is the DigitalPlanning prefix redundant given the context, will all schemas end up with the prefix?

Very on board with simplifying at this stage ⚡ agree dropping prefix and keeping Application only is less redundant and going to be easier to extend (eg PreApplication, Assessment, Consultation, SiteNotice etc)

Agreed! Will pick this up in a follow up PR shortly.

DafyddLlyr · 2024-07-16T21:31:33Z

tests/usage.test.ts

+  {
+    name: 'DigitalPlanningApplication',
+    schema: digitalPlanningApplicationSchema,
+    examples: getJSONExamples<DigitalPlanningApplication>(
+      'digitalPlanningApplication'
+    ),
+  },


New schemas will also need to be added here, and then all their examples will get picked up by tests automatically 👍

DafyddLlyr · 2024-07-16T21:34:49Z

tests/usage.test.ts

+    const ajv = addFormats(new Ajv({allowUnionTypes: true}));
+    const validate = ajv.compile(schema);


Moving the instantiation of ajv and jsonschema up to the describe() block and not repeating in it each test speeds tests up from ~55s to ~3s locally.

Huge ! Thanks for spotting that misconfiguration

jessicamcinchak

This looks great ! Reviewed as much as one can for 65+ changed files, let's get to main & fix forward anything else we spot 🙂

jessicamcinchak · 2024-07-17T07:51:39Z

scripts/build-json-examples.ts

+(async () => {
+  await walkDirectory('./examples');
+  console.log('All example files converted to JSON');
+})();


Much appreciated dev workflow improvement 👍

DafyddLlyr · 2024-07-17T08:10:31Z

Thanks @jessicamcinchak! Certainly a larger and noisier PR than intended, really appreciate the speedy review 🙌

DafyddLlyr force-pushed the dp/support-multiple-schemas branch from 7e0eb07 to a858bfd Compare July 16, 2024 12:46

DafyddLlyr commented Jul 16, 2024

View reviewed changes

DafyddLlyr added 4 commits July 16, 2024 15:49

fix: Update 'Build and move files' step to preserve new example folde…

734a036

…r structure (#202)

refactor: Build scripts for multiple schemas

8a2bfc3

feat: Walk example directories to make JSON, no longer need to import…

69fa2a8

… examples manually

feat: Update CI publish action

323e133

DafyddLlyr force-pushed the dp/support-multiple-schemas branch from 70d1b7b to 323e133 Compare July 16, 2024 14:49

DafyddLlyr commented Jul 16, 2024

View reviewed changes

feat: Walk /examples dir for tests

f5a2389

DafyddLlyr force-pushed the dp/support-multiple-schemas branch from 7fb8a41 to f5a2389 Compare July 16, 2024 21:29

DafyddLlyr commented Jul 16, 2024

View reviewed changes

docs: Update README.md

69206c1

DafyddLlyr requested a review from a team July 17, 2024 06:34

DafyddLlyr marked this pull request as ready for review July 17, 2024 06:34

jessicamcinchak approved these changes Jul 17, 2024

View reviewed changes

DafyddLlyr merged commit c7d6578 into main Jul 17, 2024
3 checks passed

DafyddLlyr deleted the dp/support-multiple-schemas branch July 17, 2024 08:11

DafyddLlyr mentioned this pull request Jul 22, 2024

chore: Rename DigitalPlanningApplication to Application #211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support multiple schema types #203

feat: Support multiple schema types #203

DafyddLlyr commented Jul 16, 2024 •

edited

Loading

DafyddLlyr Jul 16, 2024

DafyddLlyr Jul 16, 2024

DafyddLlyr Jul 16, 2024 •

edited

Loading

DafyddLlyr Jul 16, 2024 •

edited

Loading

DafyddLlyr Jul 16, 2024

jessicamcinchak Jul 17, 2024

DafyddLlyr Jul 17, 2024

DafyddLlyr Jul 16, 2024

DafyddLlyr Jul 16, 2024 •

edited

Loading

jessicamcinchak Jul 17, 2024

jessicamcinchak left a comment

jessicamcinchak Jul 17, 2024

DafyddLlyr commented Jul 17, 2024

		dirs=("digitalPlanningApplication")
		types=("DigitalPlanningApplication")

		const ajv = addFormats(new Ajv({allowUnionTypes: true}));
		const validate = ajv.compile(schema);

feat: Support multiple schema types #203

feat: Support multiple schema types #203

Conversation

DafyddLlyr commented Jul 16, 2024 • edited Loading

What does this PR do?

Changes to folder structure

Tree view

Choose a reason for hiding this comment

Bug Fixes

Choose a reason for hiding this comment

DafyddLlyr Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

DafyddLlyr Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DafyddLlyr Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DafyddLlyr commented Jul 17, 2024

DafyddLlyr commented Jul 16, 2024 •

edited

Loading

DafyddLlyr Jul 16, 2024 •

edited

Loading

DafyddLlyr Jul 16, 2024 •

edited

Loading

DafyddLlyr Jul 16, 2024 •

edited

Loading