Feature/type generator #6

jeskew · 2017-04-28T23:11:00Z

This PR includes #3 as it relies on the types defined there.

Still writing tests and working on removing the dependency on tsfmt.

…nstead of a normalized service model

chrisradek · 2017-05-12T17:31:43Z

packages/service-model/lib/Shape.ts

@@ -0,0 +1,291 @@
+import {isArrayOf} from "./isArrayOf";
+import {isObjectMapOf} from "./isObjectMapOf";
+import {XmlNamespace} from "../../types/protocol";


This should reference the @aws/types package, right?

chrisradek · 2017-05-12T17:42:33Z

packages/service-model/lib/Shape.ts

+export type Type = 'boolean'|'byte'|'timestamp'|'character'|'double'|'float'|'integer'|'long'|'short'|'string'|'blob'|'list'|'map'|'structure';
+
+interface ShapeDef {
+    readonly type: Type


Nit: semicolon missing

chrisradek · 2017-05-12T17:43:34Z

packages/service-model/lib/Shape.ts

+
+export type Type = 'boolean'|'byte'|'timestamp'|'character'|'double'|'float'|'integer'|'long'|'short'|'string'|'blob'|'list'|'map'|'structure';
+
+interface ShapeDef {


I think you'll probably need to extend this class when generating declarations (might need to make most if not all interfaces exported).

This is an abstraction layer over the service models and wouldn't be used outside of this package. I'll sprinkle comments throughout to clarify.

chrisradek · 2017-05-12T17:56:03Z

packages/service-model/lib/Shape.ts

+        && ['undefined', 'boolean'].indexOf(typeof arg.deprecated) > -1;
+}
+
+export interface Blob extends ShapeDef {


We might need to change the names of some of the exported interfaces. I'm not sure what TypeScript will do when it tries to export DOM types (Blob) or standard JavaScript classes (Boolean), when a user is also specifying that they are importing the DOM types.

Users can alias if they need to.

chrisradek · 2017-05-12T18:05:14Z

packages/service-model/lib/Shape.ts

+}
+
+export function isStructureMember(arg: any): arg is StructureMember {
+    return isMember(<StructureMember>arg) &&


Why do you have to cast <StructureMember> here? I thought since isStructureMember identifies arg as StructureMember if the method returns true, that's all that's needed.

Without the cast, the compiler was throwing errors, though I'm not sure why.

chrisradek · 2017-05-15T19:37:06Z

packages/service-model/__fixtures__/index.ts

+    shapes: {
+        ConsumedCapacity: {
+            type: 'structure',
+            members: {},


Curious, is it possible to have a structure without any members, aside from an input/output shape?

It's not explicitly forbidden, but I don't think any services have empty structures. They could be used as sigils (do one thing if an object exists; do another if it is null), but I don't think that's a common pattern.

chrisradek · 2017-05-15T19:45:52Z

packages/service-model/__tests__/ApiModel/ApiModel.ts

+    });
+
+    it(
+        'should reject objects where a "documentation" property is present and not a string',


We should probably have a test that it accepts a model with documentation as a string as well. Can it be null/undefined?

Documentation is undefined is every other test. It cannot be null.

chrisradek · 2017-05-15T19:55:04Z

packages/service-model/__tests__/ApiModel/Operation.ts

+    isOperation,
+} from "../../lib/ApiModel/Operation";
+
+describe('isHttpTraitDefinition', () => {


Imethod and requestUri are both required, right? If so, can we add a test that fails if either is missing?

Sure, but it's just exercising the same code path as the tests above. If a required parameter is not present, it is a type mismatch (undefined instead of string). Presence/absence is equivalent to any other type assertion.

chrisradek · 2017-05-15T19:57:03Z

packages/service-model/__tests__/ApiModel/Operation.ts

+    );
+
+    it(
+        'should reject objects where a "documentation" property is present and not a boolean',


There are a lot of reject cases here, which is great, but can we also include the accepts cases?

chrisradek · 2017-05-15T20:52:40Z

packages/service-model/__tests__/ApiModel/ServiceMetadata.ts

+        expect(isServiceMetadata(Object.assign(
+            {},
+            minimalValidServiceMetadata,
+            {apiVersion: 1}


apiVersion should be endpointPrefix

chrisradek · 2017-05-15T20:53:01Z

packages/service-model/__tests__/ApiModel/ServiceMetadata.ts

+        expect(isServiceMetadata(Object.assign(
+            {},
+            minimalValidServiceMetadata,
+            {endpointPrefix: 1}


endpointPrefix should be protocol

chrisradek · 2017-05-15T20:53:43Z

packages/service-model/__tests__/ApiModel/ServiceMetadata.ts

+        expect(isServiceMetadata(Object.assign(
+            {},
+            minimalValidServiceMetadata,
+            {signatureVersion: 1}


Do we also validate that the signature version is one of a set of types? (e.g. v4, v2, etc)

Yes, and we do the same for protocol. I'll add a check to make sure we're rejecting strings that are not a valid signature version or protocol.

(Side note: we are currently rejecting 'v2' and will need to get special permission to allow it.)

chrisradek · 2017-05-15T20:56:35Z

packages/service-model/__tests__/ApiModel/Shape.ts

+    });
+
+    it(
+        'should reject objects where a "documentation" property is present and not a string',


Since these fields aren't in the minimalValidStructureMember, we should have accepts cases as well.

chrisradek · 2017-05-15T20:58:35Z

packages/service-model/__tests__/ApiModel/Shape.ts

+        }
+    });
+
+    it('should return true for all other shapes', () => {


should return false?

chrisradek · 2017-05-15T21:14:59Z

packages/service-model/__tests__/ApiModel/Shape.ts

+        });
+
+        it(
+            'should reject objects where "members" is not an object map of StructureMembers',


Can you add an example where members is an object map of StructureMembers?

chrisradek · 2017-05-15T23:38:10Z

packages/service-model/__tests__/TreeModel/isReferencedByOperation.ts

+    );
+
+    it(
+        'should return true when an operation uses the shape as its input',


input -> output

chrisradek · 2017-05-15T23:38:27Z

packages/service-model/__tests__/TreeModel/isReferencedByOperation.ts

+    );
+
+    it(
+        'should return true when an operation uses the shape as its input',


input -> errors?

chrisradek · 2017-05-15T23:39:23Z

packages/service-model/__tests__/TreeModel/isReferencedByOperation.ts

+    );
+
+    it(
+        'should return false for shapes not referenced directly by operations',


So is it intended that isReferencedByOperation only looks at top-level shapes?

Right. The parser calls this function along with isMember to determine if a shape is referenced either as a member of another shape or as a shape hung onto an operation. The implementation (and usage) follow the semantics of a service model.

I should find out what the TSDoc equivalent of YARD's @api private is and use it on most of this package...

…alizedModel

…rmalizedModel

chrisradek

Feature/type generator

lock · 2019-09-26T23:07:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

Add service model parser package

a46b677

jeskew requested a review from chrisradek April 28, 2017 23:11

Rename ApiDefinition file

d854eb9

jeskew changed the title ~~[WIP] Feature/type generator~~ Feature/type generator May 1, 2017

jeskew and others added 3 commits May 6, 2017 18:38

Make the parser emit shapes and operations as defined in @aws/types i…

963df2c

…nstead of a normalized service model

Refactor parser and improve test coverage

0f02529

Do not rename error shapes when normalizing models

f06eb7a

jeskew force-pushed the feature/type-generator branch from d4f59cb to 7f953e5 Compare May 8, 2017 20:50

jeskew added 2 commits May 8, 2017 19:52

Pass enum traits on to tree model

a2efdfe

Add optional documentation property to TreeModelMember interface

82d42b3

jeskew force-pushed the feature/type-generator branch 2 times, most recently from 8412151 to 1c2ae53 Compare May 10, 2017 05:21

chrisradek reviewed May 12, 2017

View reviewed changes

Make the API model explicitly an SDK internal

a59e654

jeskew force-pushed the feature/type-generator branch from 1c2ae53 to 7a0bb12 Compare May 14, 2017 07:58

chrisradek reviewed May 15, 2017

View reviewed changes

Add more tests to ensure valid models are accepted

c0c7a1b

jeskew force-pushed the feature/type-generator branch from 7a0bb12 to 7d5d274 Compare May 16, 2017 20:15

chrisradek reviewed May 19, 2017

View reviewed changes

jeskew and others added 5 commits May 19, 2017 12:58

Add documentation for public types and mark internal types as such

cb87483

WIP commit

43de9a8

100% test coverage FTW

9f72cf6

Update serialization model generator to use TreeModel instead of Norm…

3425613

…alizedModel

Update typescript definition generator to use TreeModel instead of No…

175542b

…rmalizedModel

jeskew force-pushed the feature/type-generator branch from 7d5d274 to 175542b Compare May 19, 2017 19:59

chrisradek approved these changes May 19, 2017

View reviewed changes

Ensure generator tests are not exhausting empty shape and operation maps

7f9897e

jeskew merged commit 60f422c into master May 19, 2017

jeskew deleted the feature/type-generator branch May 27, 2017 21:42

trivikr referenced this pull request in trivikr/aws-sdk-js-v3 Dec 10, 2018

Merge pull request #6 from aws/feature/type-generator

900d46f

Feature/type generator

lock bot locked as resolved and limited conversation to collaborators Sep 26, 2019


		export type Type = 'boolean'\|'byte'\|'timestamp'\|'character'\|'double'\|'float'\|'integer'\|'long'\|'short'\|'string'\|'blob'\|'list'\|'map'\|'structure';

		interface ShapeDef {

Feature/type generator #6

Feature/type generator #6

Uh oh!

Conversation

jeskew commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

lock bot commented Sep 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!