From 8337014cd67463679731fb0d8bf2e64356b465eb Mon Sep 17 00:00:00 2001 From: Eyas Sharaiha Date: Mon, 18 May 2020 11:35:37 -0400 Subject: [PATCH 1/3] Rationalize DataType and allowString Rationalizes our "typedef" datatypes and "allowstring" into a generalized typedef mechanism. Restructures inheretence and merging so that, like allowString, these typedefs are just leaf types. This allows us to then re-support PronounceableText. --- src/transform/toClass.ts | 8 +- src/ts/class.ts | 142 ++++++++++++------------ test/baselines/stringlike_http_test.ts | 4 +- test/baselines/stringlike_https_test.ts | 4 +- test/baselines/url_test.ts | 4 +- test/ts/class_test.ts | 10 +- 6 files changed, 91 insertions(+), 81 deletions(-) diff --git a/src/transform/toClass.ts b/src/transform/toClass.ts index 4b306de..5514e52 100644 --- a/src/transform/toClass.ts +++ b/src/transform/toClass.ts @@ -81,13 +81,13 @@ function ForwardDeclareClasses(topics: readonly TypedTopic[]): ClassMap { continue; } else if (!IsNamedClass(topic)) continue; + const cls = new Class(topic.Subject); const allowString = wellKnownStrings.some(wks => wks.equivTo(topic.Subject) ); - classes.set( - topic.Subject.toString(), - new Class(topic.Subject, allowString) - ); + if (allowString) cls.addTypedef('string'); + + classes.set(topic.Subject.toString(), cls); } return classes; diff --git a/src/ts/class.ts b/src/ts/class.ts index 6730951..d930245 100644 --- a/src/ts/class.ts +++ b/src/ts/class.ts @@ -69,24 +69,27 @@ export type ClassMap = Map; */ export class Class { private _comment?: string; + private _typedef?: string; private readonly children: Class[] = []; - private readonly parents: Class[] = []; + private readonly _parents: Class[] = []; private readonly _props: Set = new Set(); private readonly _enums: Set = new Set(); private readonly _supersededBy: Set = new Set(); - private inheritsDataType(): boolean { - if (this instanceof Builtin) return true; - for (const parent of this.parents) { - if (parent instanceof Builtin || parent.inheritsDataType()) { - return true; - } - } - return false; + private allParents(): readonly Class[] { + return this._parents; + } + private namedParents(): readonly string[] { + return this._parents + .map(p => p.baseName()) + .filter((name): name is string => !!name); } isNodeType(): boolean { - return !this.inheritsDataType(); + if (this instanceof Builtin) return false; + if (this._props.size > 0) return true; + + return this.allParents().every(n => n.isNodeType()); } get deprecated() { @@ -101,6 +104,13 @@ export class Class { return this._comment ? `${this._comment}\n${deprecated}` : deprecated; } + protected get typedefs(): string[] { + const parents = this.allParents().flatMap(p => p.typedefs); + return Array.from( + new Set(this._typedef ? [this._typedef, ...parents] : parents) + ).sort(); + } + private properties() { return Array.from(this._props.values()).sort((a, b) => CompareKeys(a.key, b.key) @@ -119,23 +129,24 @@ export class Class { ); } - private get allowString(): boolean { - return ( - this._allowStringType || this.parents.some(parent => parent.allowString) - ); - } - - protected baseName(): string { + private baseName(): string | undefined { // If Skip Base, we use the parent type instead. if (this.skipBase()) { - assert(this.parents.length === 1); - return this.parents[0].baseName(); + if (this.namedParents().length === 0) return undefined; + assert(this.namedParents().length === 1); + return this.namedParents()[0]; } return toClassName(this.subject) + 'Base'; } - protected leafName() { + private leafName(): string | undefined { + // If the leaf has no node type and doesn't refer to any parent, + // skip defining it. + if (!this.isNodeType() && this.namedParents().length === 0) { + return undefined; + } + return toClassName(this.subject) + 'Leaf'; } @@ -143,10 +154,7 @@ export class Class { return toClassName(this.subject); } - constructor( - readonly subject: TSubject, - private readonly _allowStringType: boolean - ) {} + constructor(readonly subject: TSubject) {} add( value: {Predicate: TPredicate; Object: TObject}, classMap: ClassMap @@ -169,7 +177,7 @@ export class Class { const parentClass = classMap.get(s.subClassOf.toString()); if (parentClass) { - this.parents.push(parentClass); + this._parents.push(parentClass); parentClass.children.push(this); } else { throw new Error( @@ -196,6 +204,16 @@ export class Class { return false; } + + addTypedef(typedef: string) { + if (this._typedef) { + throw new Error( + `Class ${this.subject.href} already has typedef ${this._typedef} but we're also adding ${typedef}` + ); + } + this._typedef = typedef; + } + addProp(p: Property) { this._props.add(p); } @@ -204,8 +222,8 @@ export class Class { } private skipBase(): boolean { - if (this.inheritsDataType()) return true; - return this.parents.length === 1 && this._props.size === 0; + if (!this.isNodeType()) return true; + return this.namedParents().length === 1 && this._props.size === 0; } private baseNode( @@ -216,8 +234,8 @@ export class Class { return undefined; } - const parentTypes = this.parents.map(parent => - createTypeReferenceNode(parent.baseName(), []) + const parentTypes = this.namedParents().map(p => + createTypeReferenceNode(p, []) ); const parentNode = parentTypes.length === 0 @@ -250,32 +268,30 @@ export class Class { const baseNode = this.baseNode(skipDeprecatedProperties, context); if (!baseNode) return undefined; + const baseName = this.baseName(); + assert(baseName, 'If a baseNode is defined, baseName must be defined.'); return createTypeAliasDeclaration( /*decorators=*/ [], /*modifiers=*/ [], - this.baseName(), + baseName, /*typeParameters=*/ [], baseNode ); } - private leafDecl(context: Context): TypeAliasDeclaration | undefined { - // If we inherit from a DataType (~= a Built In), then the type is _not_ - // represented as a node. Skip the leaf type. - // - // TODO: This should probably be modeled differently given the advent of 'PronounceableText'. - // - // That is, 'PronounceableText' inherits 'Text', but does not _extend_ string per se; - // string, in Text, is a leaf. - // - // This breaks down because, e.g. 'URL' also inherits 'Text', but is string as well. - if (this.inheritsDataType()) { - return undefined; - } + protected leafDecl(context: Context): TypeAliasDeclaration | undefined { + const leafName = this.leafName(); + if (!leafName) return undefined; + const baseName = this.baseName(); + // Leaf is missing if !isNodeType || namedParents.length == 0 + // Base is missing if !isNodeType && namedParents.length == 0 && numProps == 0 + // + // so when "Leaf" is present, Base will always be present. + assert(baseName, 'Expect baseName to exist when leafName exists.'); const baseTypeReference = createTypeReferenceNode( - this.baseName(), + baseName, /*typeArguments=*/ [] ); @@ -287,7 +303,7 @@ export class Class { return createTypeAliasDeclaration( /*decorators=*/ [], /*modifiers=*/ [], - this.leafName(), + leafName, /*typeParameters=*/ [], thisType ); @@ -301,22 +317,15 @@ export class Class { createTypeReferenceNode(child.className(), /*typeArguments=*/ []) ); - // 'String' is a valid Type sometimes, add that as a Child if so. - if (this.allowString) { - children.push(createTypeReferenceNode('string', /*typeArguments=*/ [])); - } - - const leafTypeReference = createTypeReferenceNode( - // If we inherit from a DataType (~= a Built In), then the type is _not_ - // represented as a node. Skip the leaf type. - // - // TODO: This should probably be modeled differently given the advent of 'PronounceableText'. - // See the note in 'leafDecl' for more details. - this.inheritsDataType() ? this.baseName() : this.leafName(), - /*typeArguments=*/ [] + // A type can have a valid typedef, add that if so. + children.push( + ...this.typedefs.map(t => createTypeReferenceNode(t, /*typeArgs=*/ [])) ); - return [leafTypeReference, ...children]; + const upRef = this.leafName() || this.baseName(); + return upRef + ? [createTypeReferenceNode(upRef, /*typeArgs=*/ []), ...children] + : children; } private totalType(context: Context, skipDeprecated: boolean): TypeNode { @@ -406,22 +415,15 @@ export class Builtin extends Class {} * in JSON-LD and JavaScript as a typedef to a native type. */ export class AliasBuiltin extends Builtin { - constructor(url: string, private readonly equivTo: string) { - super(UrlNode.Parse(url), false); - } - - nonEnumType(): TypeNode[] { - return [createTypeReferenceNode(this.equivTo, [])]; - } - - protected baseName() { - return this.subject.name; + constructor(url: string, equivTo: string) { + super(UrlNode.Parse(url)); + this.addTypedef(equivTo); } } export class DataTypeUnion extends Builtin { constructor(url: string, readonly wk: Builtin[]) { - super(UrlNode.Parse(url), false); + super(UrlNode.Parse(url)); } toNode(): DeclarationStatement[] { diff --git a/test/baselines/stringlike_http_test.ts b/test/baselines/stringlike_http_test.ts index ab5bdfa..641db39 100644 --- a/test/baselines/stringlike_http_test.ts +++ b/test/baselines/stringlike_http_test.ts @@ -71,7 +71,7 @@ test(`baseine_${basename(__filename)}`, async () => { \\"@id\\": string; }; - export type Text = string; + export type Text = URL | string; type EntryPointLeaf = { \\"@type\\": \\"EntryPoint\\"; @@ -115,7 +115,7 @@ test(`baseine_${basename(__filename)}`, async () => { } & ThingBase; export type Thing = ThingLeaf | EntryPoint | Organization | Person | Place | Quantity; - export type URL = Text; + export type URL = string; " `); diff --git a/test/baselines/stringlike_https_test.ts b/test/baselines/stringlike_https_test.ts index 833d6cb..9f8cea2 100644 --- a/test/baselines/stringlike_https_test.ts +++ b/test/baselines/stringlike_https_test.ts @@ -71,7 +71,7 @@ test(`baseine_${basename(__filename)}`, async () => { \\"@id\\": string; }; - export type Text = string; + export type Text = URL | string; type EntryPointLeaf = { \\"@type\\": \\"EntryPoint\\"; @@ -115,7 +115,7 @@ test(`baseine_${basename(__filename)}`, async () => { } & ThingBase; export type Thing = ThingLeaf | EntryPoint | Organization | Person | Place | Quantity; - export type URL = Text; + export type URL = string; " `); diff --git a/test/baselines/url_test.ts b/test/baselines/url_test.ts index 613bf8a..3a790e6 100644 --- a/test/baselines/url_test.ts +++ b/test/baselines/url_test.ts @@ -46,10 +46,10 @@ test(`baseine_${basename(__filename)}`, async () => { \\"@id\\": string; }; - export type Text = string; + export type Text = URL | string; /** Data type: URL. */ - export type URL = Text; + export type URL = string; " `); diff --git a/test/ts/class_test.ts b/test/ts/class_test.ts index dcd39e3..1481701 100644 --- a/test/ts/class_test.ts +++ b/test/ts/class_test.ts @@ -70,6 +70,14 @@ describe('Class', () => { }); }); + it("can't add typedef twice", () => { + const cls = makeClass('https://schema.org/Person'); + cls.addTypedef('string'); + expect(() => cls.addTypedef('Foo')).toThrowError( + 'already has typedef string' + ); + }); + describe('toNode', () => { it('by default (no parent)', () => { // A class with no parent has a top-level "@id" @@ -346,7 +354,7 @@ describe('Sort(Class, Class)', () => { // Can be same as less specific builtins. expect( Sort( - new Builtin(UrlNode.Parse('https://schema.org/Boo'), false), + new Builtin(UrlNode.Parse('https://schema.org/Boo')), new AliasBuiltin('https://schema.org/Boo', 'Text') ) ).toBe(0); From 9cd5bb33f5a37b89f0d2f164eea8160b07623942 Mon Sep 17 00:00:00 2001 From: Eyas Sharaiha Date: Mon, 18 May 2020 12:07:03 -0400 Subject: [PATCH 2/3] Add test for better coverage for nested datatypes. --- test/baselines/nested_datatype_test.ts | 110 +++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 test/baselines/nested_datatype_test.ts diff --git a/test/baselines/nested_datatype_test.ts b/test/baselines/nested_datatype_test.ts new file mode 100644 index 0000000..9777710 --- /dev/null +++ b/test/baselines/nested_datatype_test.ts @@ -0,0 +1,110 @@ +/** + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @fileoverview Baseline tests are a set of tests (in tests/baseline/) that + * correspond to full comparisons of a generate .ts output based on a set of + * Triples representing an entire ontology. + */ +import {basename} from 'path'; + +import {inlineCli} from '../helpers/main_driver'; + +test(`baseine_${basename(__filename)}`, async () => { + const {actual} = await inlineCli( + ` + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . + . +`, + ['--ontology', `https://fake.com/${basename(__filename)}.nt`] + ); + + expect(actual).toMatchInlineSnapshot(` + "/** Used at the top-level node to indicate the context for the JSON-LD objects used. The context provided in this type is compatible with the keys and URLs in the rest of this generated file. */ + export type WithContext = T & { + \\"@context\\": \\"https://schema.org\\"; + }; + + type SchemaValue = T | readonly T[]; + type IdReference = { + /** IRI identifying the canonical address of this object. */ + \\"@id\\": string; + }; + + export type Text = PronounceableText | URL | string; + + type ArabicTextBase = PronounceableTextBase & { + \\"arabicPhoneticText\\"?: SchemaValue; + }; + type ArabicTextLeaf = { + \\"@type\\": \\"ArabicText\\"; + } & ArabicTextBase; + export type ArabicText = ArabicTextLeaf | string; + + type EnglishTextLeaf = { + \\"@type\\": \\"EnglishText\\"; + } & PronounceableTextBase; + export type EnglishText = EnglishTextLeaf | string; + + export type FancyURL = string; + + type PronounceableTextBase = Partial & { + \\"phoneticText\\"?: SchemaValue; + }; + type PronounceableTextLeaf = { + \\"@type\\": \\"PronounceableText\\"; + } & PronounceableTextBase; + export type PronounceableText = PronounceableTextLeaf | ArabicText | EnglishText | string; + + type ThingBase = Partial & { + \\"name\\"?: SchemaValue; + \\"pronunciation\\"?: SchemaValue; + \\"website\\"?: SchemaValue; + }; + type ThingLeaf = { + \\"@type\\": \\"Thing\\"; + } & ThingBase; + export type Thing = ThingLeaf; + + export type URL = FancyURL | string; + + " + `); +}); From 35c2c0db32fff45f3d7ab1160fad257acee1d289 Mon Sep 17 00:00:00 2001 From: Eyas Sharaiha Date: Mon, 18 May 2020 12:14:32 -0400 Subject: [PATCH 3/3] Fix test still referencing allowString. --- test/helpers/make_class.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/helpers/make_class.ts b/test/helpers/make_class.ts index d668085..b22312e 100644 --- a/test/helpers/make_class.ts +++ b/test/helpers/make_class.ts @@ -16,8 +16,8 @@ import {UrlNode} from '../../src/triples/types'; import {Class, ClassMap} from '../../src/ts/class'; -export function makeClass(url: string, allowString = false): Class { - return new Class(UrlNode.Parse(url), allowString); +export function makeClass(url: string): Class { + return new Class(UrlNode.Parse(url)); } export function makeClassMap(...classes: Class[]): ClassMap {