chore: improve JSDoc annotations #60
Conversation
types/schema/data/Property.ts
Outdated
| */ | ||
| export interface LondonProperty extends UKProperty { | ||
| region: 'London'; |
There was a problem hiding this comment.
this makes for a bit of a messy union type that's 'London' | string - better suggestions for handling / communicating what I'm trying to get at here?
should i just type as a proper enum with the 9 possible regions returned by https://www.planning.data.gov.uk/dataset/region? i don't think these should change often?
There was a problem hiding this comment.
This is a good question. There's basically two ways of doing this that I can think of which better describe the type we intend to be used here.
The first would be something like -
type UKRegion = string extends "London" ? never : string;This is pretty neat as TS "trick" but basically meaningless in terms of the schema.
I think a better approach would be to define a UKRegion string union type of all 9 just as you suggest, and reference the Planning Data link as the source of these 9 strings in a comment or JSDoc.
We can then end up with Exclude<UKRegion, "London"> and Extract<UKRegion, "London"> (this second one is also the same as the string literal type "London" but is maybe more meaningful?).
Hopefully they would compile correctly into JSON schema!
There was a problem hiding this comment.
One thing I'm super keen to discuss further with Planning Data is them being the custodian of these data dictionaries - I think this is a very meaningful and potentially quick win which may solve #51 in a satisfying way also.
There was a problem hiding this comment.
Thanks! Updated to type out an enum with the 9 values - only glitch to the original suggestion of Exclude... and Extract is that a LondonProperty currently extends a UKProperty which then means non-intersecting type error.
So I've gone with UKProperty having full UKRegion type as initial compromise here - still more precise than string!
There was a problem hiding this comment.
Ah ok - I think to make this work then we'd need an abstract (Property?) base interface and then extend both UKProperty and LondonProperty from this probably.
What you have here though is a totally fair and sensible compromise though 👍
types/schema/data/Property.ts
Outdated
| */ | ||
| export interface LondonProperty extends UKProperty { | ||
| region: 'London'; |
There was a problem hiding this comment.
This is a good question. There's basically two ways of doing this that I can think of which better describe the type we intend to be used here.
The first would be something like -
type UKRegion = string extends "London" ? never : string;This is pretty neat as TS "trick" but basically meaningless in terms of the schema.
I think a better approach would be to define a UKRegion string union type of all 9 just as you suggest, and reference the Planning Data link as the source of these 9 strings in a comment or JSDoc.
We can then end up with Exclude<UKRegion, "London"> and Extract<UKRegion, "London"> (this second one is also the same as the string literal type "London" but is maybe more meaningful?).
Hopefully they would compile correctly into JSON schema!
types/schema/data/Property.ts
Outdated
| */ | ||
| export interface LondonProperty extends UKProperty { | ||
| region: 'London'; |
There was a problem hiding this comment.
One thing I'm super keen to discuss further with Planning Data is them being the custodian of these data dictionaries - I think this is a very meaningful and potentially quick win which may solve #51 in a satisfying way also.
Small tidy ups while doing presentation prep & comparing to OneApp