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

Integration with tsd-jsdoc for generating typings #6767

Merged
merged 4 commits into from Jul 25, 2018
Merged

Integration with tsd-jsdoc for generating typings #6767

merged 4 commits into from Jul 25, 2018

Conversation

bampakoa
Copy link
Contributor

@bampakoa bampakoa commented Jul 3, 2018

References #5717

This PR contains modifications in the jsdoc block tags so that we can nicely integrate with tsd-jsdoc and create a typings file for the library. Initially we are adding missing jsdoc block tags in order for tsd-jsdoc to have a first time pass from each subfolder of Sources directory and compile successfully.

  • Core
  • Renderer
  • Scene

@cesium-concierge
Copy link

Signed CLA is on file.

@bampakoa, thanks for the pull request! Maintainers, we have a signed CLA from @bampakoa, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Jul 3, 2018

Thanks for the pull request @bampakoa! These changes look good so far. Let us know when this is ready for a full review

@bampakoa
Copy link
Contributor Author

bampakoa commented Jul 10, 2018

@hpinkos I have added the namespace block in AutomaticUniforms file as stated in Using @alias for an object literal example. I am not really sure if namespace should be added, but something was missing there and I think namespace is a good fit. What do you think?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 10, 2018

Hmm, @namespace seems like it might be the right thing to do there but I'm not sure either. @mramato what do you think?

@bampakoa
Copy link
Contributor Author

@hpinkos I have noticed that Cesium uses internalConstructor block tag in various pieces of the code, which is not part of the standard JSDoc syntax? What is this all about?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 23, 2018

@bampakoa this is one of our custom tags. You can see the others here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Tools/jsdoc/cesiumTags.js

internalConstructor is an alias of class. We use it to denote things that shouldn't be instantiated by the end user. For example Billboard has a constructor, but you should never call new Billboard(). Instead, you create one using billboardCollection.add({ billboardProperties }).

@bampakoa
Copy link
Contributor Author

bampakoa commented Jul 24, 2018

@hpinkos I added also class block tag where internalConstructor is used so that jsdoc can differentiate the actual tag.

PR is now ready for review. There are a lot of warnings yet but tsd-jsdoc is now running successfully with Cesium repo.

@bampakoa bampakoa changed the title WIP: Integration with tsd-jsdoc for generating typings Integration with tsd-jsdoc for generating typings Jul 24, 2018
@@ -599,7 +599,7 @@ define([
* calling <code>update</code>.
*
* @exception {DeveloperError} This object was destroyed, i.e., destroy() was called.
*
* @memberOf {LabelCollection.prototype}
Copy link
Contributor

Choose a reason for hiding this comment

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

These shouldn't be needed. We don't do this for our prototype functions in any other class

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 have seen it in various places of the code https://github.com/AnalyticalGraphicsInc/cesium/search?p=4&q=memberOf&type=Code and I thought that tsd-jsdoc was failing due to this one. Is there any similar file such as LabelCollection that I could check out instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we use @memberof for properties added to prototype via defineProperties but shouldn't need it for class methods

Copy link
Contributor

Choose a reason for hiding this comment

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

BillboardCollection is very similar to LabelCollection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification 😃 I will update the PR accordingly.

@hpinkos
Copy link
Contributor

hpinkos commented Jul 24, 2018

I just had that one comment. These changes look fine to me. Does anyone else want to take a look? I'll merge this by the end of the week if no one else has comments

@bampakoa
Copy link
Contributor Author

@hpinkos the changes in the PR are available for review

@hpinkos
Copy link
Contributor

hpinkos commented Jul 25, 2018

Everything looks good @bampakoa! I can confirm the documentation builds correctly.

@hpinkos
Copy link
Contributor

hpinkos commented Jul 25, 2018

I'll add a comment to CHANGES.md in master

@hpinkos hpinkos merged commit dbb3fd4 into CesiumGS:master Jul 25, 2018
@bampakoa
Copy link
Contributor Author

I would have done it, as @cesium-concierge notified me about it, but I thought it was not meant to be an actual change 🤔

@hpinkos
Copy link
Contributor

hpinkos commented Jul 25, 2018

No worries 😄 I just decided to do it to save some additional back and forth. I wasn't sure if it warranted a line in CHANGES initially, but I think other users using TypeScript will be interested in this so I decided to add it. Thanks again for the contribution!

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.

None yet

3 participants