Skip to content

Minor argument type fixes in JSDoc #10899

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

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Nov 12, 2022

Fixes #10801 (which I had kept open in a tab for ~2 months now...)

I assume that there are (potentially many) similar issues like that, with different levels of severity

  • types that are plainly wrong
  • types that are not found
  • types that are capitalized wrong (e.g. {Number} instead of {number} - whatever is "right" there, it's certainly not consistent right now - see https://jsdoc.app/tags-type.html )

In order to find them, one should use a dedicated tool (maybe one of the tools that have once been used in the first experiments for generating TypeScript bindings?).

But these are the ones that I noticed in the issue.

@cesium-concierge
Copy link

Thank you so much for the pull request @javagl! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@javagl
Copy link
Contributor Author

javagl commented Nov 12, 2022

The CONTRIBUTORS.md update can be skipped for such a fix.

Regarding the CHANGES.md... I'm not sure: Strictly speaking, this is a breaking change, namely for the generated TypeScript bindings (and that's where I noticed that issue in the first place...)

@ggetz
Copy link
Contributor

ggetz commented Nov 14, 2022

Thanks for the fix, @javagl!

In order to find them, one should use a dedicated tool (maybe one of the tools that have once been used in the first experiments for generating TypeScript bindings?).

The tool we current use does issue some warnings, but unfortunately due to an issue with the tool, the output is very noisy. Happy to hear if you have alternative suggestions!

Regarding the CHANGES.md... I'm not sure: Strictly speaking, this is a breaking change, namely for the generated TypeScript bindings (and that's where I noticed that issue in the first place...)

Yes, there should be an item for this in CHANGES.md for this exact reason. However, we have put previous items like this under "Fixes" rather than "Breaking Changes".

@javagl
Copy link
Contributor Author

javagl commented Nov 21, 2022

Similar to #10912 (comment) , I wasn't sure about the form for the CHANGES.md update. I guess the approach is to just add a section with the new version (and a somewhat "arbitrary" date - this has to be checked during the release anyhow). I have updated CHANGES.md accordingly. The responsibility for resolving merge conflicts here will likely be determined by the merge order, but consider this PR as "low priority" compared to others.

@ggetz
Copy link
Contributor

ggetz commented Dec 7, 2022

Thanks @javagl!

@ggetz ggetz merged commit 80d4aba into CesiumGS:main Dec 7, 2022
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.

Wrong argument type definition in Matrix multiplyByScale functions
3 participants