-
Notifications
You must be signed in to change notification settings - Fork 3.7k
docs: document classes implementing GeometryFactory #13131
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
Conversation
|
Thank you for the pull request, @MohammadShujaullah! Welcome to the Cesium community! In order for us to review your PR, please complete the following steps:
Review Pull Request Guidelines to make sure your PR gets accepted quickly. |
e897d7e to
b63382d
Compare
|
@cla-bot check |
jjspace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @MohammadShujaullah, just a couple small comments
| * Base class for all geometry creation utility classes that can be passed to {@link GeometryInstance} | ||
| * for asynchronous geometry creation. | ||
| * Describes a geometry type that can be converted into a {@link Geometry}. | ||
| * <p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need to add the <p> tags yourself. This should be handled by JSDoc itself AFAIK.
| * directly. | ||
| * </p> | ||
| * | ||
| * @alias GeometryFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this needs an alias since it matches the function name itself
| * @see SphereGeometry | ||
| * @see SphereOutlineGeometry | ||
| * @see WallGeometry | ||
| * @see WallOutlineGeometry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda on the fence about the usefullness of this simply because it's such a big list. I could see it easily going out of sync with the actual classes. I don't think there's been much change in this area so maybe it doesn't matter.
@donmccurdy or @javagl this is sorta related to recent discussions we've had around classes and inheritance. Any thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @MohammadShujaullah for compiling the list! However I think I agree with @jjspace, I'd (personally) be hesitant to commit to maintaining such a list by hand.
I can't speak for every code editor, but using TypeScript language services in Zed, there are "Find All References" and "Go to Implementation" tools built into the editor. These don't work on GeometryFactory for me today (TS language service limitations with JSDoc probably), unless I either:
- (a) add
@extends GeometryFactoryannotations to the implementations, and also add type-only imports as described in Types: Enable incremental JSDoc type checking #13104 - (b) convert to ES6 classes
Getting that information out of the language server, and into published docs, might require more steps — if that's the goal. But at least we'd have confidence that the list will remain up to date.
If we do merge #13104, adding the missing @extends X or @implements X annotations could be a good next step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion on this particular case.
Pro:
- Consistency: Implementing classes are already listed in other cases (.e.g https://cesium.com/learn/cesiumjs/ref-doc/Property.html )
- Stability: It will not change. Or is anyone up for creating a new implementation of
GeometryFactory?
Con:
- Maintenance (but that's a weak point, given the 'Stability' above)
- It may become obsolete with better tooling around TS/JSDoc (but that may take months or years...)
Some of the back-and-forth links are currently broken anyhow (e.g. InterpolationAlgorithm or Packable - see #11749 (comment) ). As long as we don't have a clean(er) concept of visibility and automatisms for generating such links, I think that adding the list here is OK, and be it only to know into which classes we'll have to insert @extends GeometryFactory after the corresponding tooling was set up 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input @donmccurdy and @javagl. I think given the stability of this area which hasn't changed much I'm going to lean towards "any info is better than no info" for now. These can stay.
We should definitely look into improving this area in the future since things like this should be automated. This may come "free" or easier after some of the class and/or TS refactoring @donmccurdy mentioned but that's far outside the scope for this PR.
Fixes #12827
– Document classes implementing the
GeometryFactoryinterface.Description
This PR improves the documentation of
GeometryFactoryand its usage:GeometryFactoryis an abstract interface-like type implemented by geometry description classes with a staticcreateGeometrymethod.@seelinks inGeometryFactoryto the main implementing geometry classes.GeometryInstance.options.geometrydocs to explainGeometryvsGeometryFactory(e.g.RectangleGeometry,EllipsoidGeometry) and howcreateGeometryis used.Testing plan
npm test(or tests recommended in CONTRIBUTING)Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change