Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typeArguments to ReferenceType.toString() #396

Merged
merged 1 commit into from
Feb 15, 2017
Merged

Add typeArguments to ReferenceType.toString() #396

merged 1 commit into from
Feb 15, 2017

Conversation

jiayihu
Copy link
Contributor

@jiayihu jiayihu commented Jan 29, 2017

First of all good job for the hard work on this project! ❤️
I'm using the Converter API to generate custom documentation for Angular components and I noticed that typeArguments are missing when calling .toString() on a ReferenceType.

Copy link
Collaborator

@aciccarello aciccarello left a comment

Choose a reason for hiding this comment

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

LGTM, with some possible suggestions.

@@ -17,7 +17,7 @@ export class ReferenceType extends Type
* If the symbol cannot be found cause it's not part of the documentation this
* can be used to represent the type.
*/
name:string;
name:string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a space was accidentally added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thought that my editor removed a trailing space :D Will fix it!

@@ -115,10 +115,15 @@ export class ReferenceType extends Type
* Return a string representation of this type.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be helpful to add an example output string to the method docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I was also looking for some test for the class but I didn't it. Is there any? Thanks for your review (:

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're welcome. I don't think there is currently a test set up. The travis build seems to have run fine.

@jiayihu
Copy link
Contributor Author

jiayihu commented Feb 1, 2017

I updated after your suggestions. I added also a commit because many test files were updated by, I suppose, a postinstall script with the new Typedoc API domain.

@aciccarello
Copy link
Collaborator

Great! Having the example output helps clarify what is going on.

I'm surprised that the domain wasn't changed before. Ideally that would have been done in a separate PR.

@jiayihu
Copy link
Contributor Author

jiayihu commented Feb 1, 2017

No problem, I removed the last commit (:

@jiayihu
Copy link
Contributor Author

jiayihu commented Feb 15, 2017

Any update? Are you @aciccarello allowed to merge PRs? You're helping a lot in issues too, would be sad to leave the project inactive.

@blakeembrey blakeembrey merged commit 26fbd15 into TypeStrong:master Feb 15, 2017
@jiayihu
Copy link
Contributor Author

jiayihu commented Feb 15, 2017

@blakeembrey thanks for merging (: do you have any thoughts about adding collaborators to the project to keep it alive if you don't have much time?

@aciccarello
Copy link
Collaborator

@jiayihu We've had a few conversations on #256 and #378. I do think that we could do more in inviting people to be involved. Maybe we could add some information to the readme.

@blakeembrey
Copy link
Member

If someone is willing to take on the work, I'm certainly happy to add them as a maintainer and collaborator. I added @aciccarello for that reason 😄 He also linked to the related issues which should shed more light - I do hope to give this project to someone who can take it to the right place eventually, since that person is not currently me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants