Skip to content
This repository has been archived by the owner on Oct 24, 2021. It is now read-only.

add support for parameterised types #60

Merged
merged 4 commits into from
Jan 9, 2018

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jan 8, 2018

Fixes #58.
Fixes #59.


Just a quick fix.

Haven't really been in the generator much until now so if this is way off how it should be implemented, do let me know. Happy to discard it or redo it.

Is there a way we can default to any if we don't know the type? So if someone does NonExistent<number>, we would produce any? or should we just assume it exists?

* @template T
* @constructor
*/
function MyParameterizedType() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably pointless at the min seeing as we don't check if named types exist or not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I would just drop this, not necessary.

Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't really been in the generator much until now so if this is way off how it should be implemented, do let me know. Happy to discard it or redo it.

Looks great! Same way I would have done it.

Did you run npm run test:make-goldens? If there were some existing types in Polymer that improve because of this feature, they would show up in your diff.

Is there a way we can default to any if we don't know the type? So if someone does NonExistent, we would produce any? or should we just assume it exists?

Let's just assume it exists.

src/ts-ast.ts Outdated
itemTypes: Type[];
name: string;

constructor(name: string, ...itemTypes: Type[]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop the variadic itemTypes param and just take a regular array.

* @template T
* @constructor
*/
function MyParameterizedType() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I would just drop this, not necessary.

@43081j
Copy link
Contributor Author

43081j commented Jan 9, 2018

this is updated now.

i did run make-goldens the first time around but we didn't seem to pick up any new types, only the custom ones i had added.

@43081j
Copy link
Contributor Author

43081j commented Jan 9, 2018

i've also gone ahead and fixed #59 seeing as its in the same piece of code im adding here (just two rename entries).

i did a quick ctrl+f through the typescript lib and didn't find any other *Of<T> like names.

@aomarks aomarks merged commit 178fa3d into Polymer:master Jan 9, 2018
@aomarks
Copy link
Member

aomarks commented Jan 9, 2018

i've also gone ahead and fixed #59 seeing as its in the same piece of code im adding here (just two rename entries).

Looks great, merged. Thanks!

@43081j 43081j deleted the parameterised-types branch January 9, 2018 18:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants