Document inheritance hierarchy

[icholy]

You're currently using @see tags to point from the parent to the child but not linking the child to the parent in any way.

This is problem for me while generating TypeScript definition files.

class Appearance {
    material: Material;
    translucent: boolean;
    vertexShaderSource: string;
    fragmentShaderSource: string;
    renderState: any;
    closed: boolean;
    constructor(options?: { translucent?: boolean; closed?: boolean; material?: Material; vertexShaderSource?: string; fragmentShaderSource?: string; renderState?: RenderState });
    getFragmentShaderSource(): string;
    isTranslucent(): boolean;
    getRenderState(): any;
}

class MaterialAppearance {
    material: Material;
    translucent: boolean;
    vertexShaderSource: string;
    fragmentShaderSource: string;
    renderState: any;
    closed: boolean;
    materialSupport: MaterialAppearance.MaterialSupport;
    vertexFormat: VertexFormat;
    flat: boolean;
    faceForward: boolean;
    constructor(options?: { flat?: boolean; faceForward?: boolean; translucent?: boolean; closed?: boolean; materialSupport?: MaterialAppearance.MaterialSupport; material?: Material; vertexShaderSource?: string; fragmentShaderSource?: string; renderState?: RenderState });
    getFragmentShaderSource();
    isTranslucent(): boolean;
    getRenderState(): any;
}

MaterialAppearance should extend Appearance but I have no way of figuring that out from the annotations in your source.

jsdoc has a an @extends tag which can be used to document the relationship between a parent and child class.

Here's what I looks like in the docs

Would you consider accepting a change like this?

[pjcozzi]

Thanks @icholy.

Anyone have thoughts here?

This is fine with me, but there are a lot more places we would want it, e.g., TerrainProvider, ImageryProvider, etc. Is there also an @implements?

[icholy]

@pjcozzi if I get the thumbs up, I'll go and update all of them. I just didn't want to waste my time in case it wasn't something that was going to get accepted.

Yeah, there are @interface and @implements tags http://usejsdoc.org/tags-implements.html

[pjcozzi]

I'm OK with this. @mramato @kring any thoughts?

[mramato]

This is something I've actually been meaning to look into, so thanks @icholy for getting the ball rolling.

Since Cesium doesn't use actual inheritance, I think @interface and @implements is the right thing to use in our code base. It also looks like jsDoc will automatically use the default documentation on the interface object if it's not overridden on the implementor. This should let us delete a bunch of duplicate documentation littered throughout the code base.

I think making this change one set of classes at a time would be the way to do it, so if you want to change Appearance to @interface and @implements as well as see if there is duplicate doc that can be removed, that would be a good test case. Once we're happy with that, we can identify the other interfaces and update them and their implementations as well.

[icholy]

@mramato at first I was thinking the same thing. But take a look at this. Looks a lot like inheritance to me.

[mramato]

@mramato at first I was thinking the same thing. But take a look at this. Looks a lot like inheritance to me.

That is not actually inheritance, it's just code sharing. Inheritance would mean that Appearance would actually be in the prototype chain for MaterialAppearance, but it's not. All that code is doing is reusing the function already written in Appearance.

[icholy]

Meh, tomato, tomato. You're not using the standard mechanism, but the end result is pretty similar. I agree that it's not a perfect fit, but I think Appearance is much closer to a base class than an interface (interfaces don't define behaviour).

However, it won't really matter if it's an interface or class in the typescript definitions. I'll update it to use interfaces.

[icholy]

@mramato seems like your jsdoc template doesn't support interfaces. I stripped down Appearance to the bare minimum and nothing is getting generated.

edit: to clarify, the whole Appearance page is missing and it's not listed in the index.

[mramato]

Meh, tomato, tomato. You're not using the standard mechanism, but the end result is pretty similar.

No, the result is very different, it's only superficial on the surface. If this was really inheritance, the functions would not need to be defined at all in the object implementing them because they would get them automatically.

seems like your jsdoc template doesn't support interfaces

This may be true, but I also noticed you deleted a bunch of code in the Appearance class, the only thing you should have had to to was add the @interface attribute, everything else needs to stay. Does that still result in it not showing up?

[icholy]

Yeah that's what I tried first. After that didn't work I deleted everything in Appearance to see if it was some of the other annotations messing with it.

[mramato]

Okay, I'll take a look at this if I get a chance. Modifying our template is definitely an option, but I won't know how easy/hard it is without looking more into it.

[icholy]

@mramato are the templates a modified version of a publicly available theme? Or is it written from scratch for caesium?

[mramato]

@mramato are the templates a modified version of a publicly available theme? Or is it written from scratch for caesium?

I'm honestly not sure. I think it was at some point, but it was a 2.x version of jsDoc. It's in Tools\jsdoc\cesium_template if you want to take a look or play with it.

[shunter]

I created the current template when we upgraded to jsDoc 3.3 which was PR #1745

The template was based on the default at the time, but I ended up making huge changes to make it match our desired output.

[icholy]

@shunter I've made several attempts at this but I'm not getting anywhere. What are the odds you could update the template with support for interfaces?

[icholy]

@mramato soo uhh, can I just do them as base classes instead of interfaces? Better than nothing.

[icholy]

@mramato this has been a blocker for generating typescript definitions for almost a year now. I want to get this conversation going again.

[pjcozzi]

@mramato can @icholy move forward with this, #2879 (comment)?

@mramato soo uhh, can I just do them as base classes instead of interfaces? Better than nothing.

[ghost]

@mramato Can we please get this merged... I need typescript definitions... 😢