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

CesiumMath vs Math naming ambiguity #6233

Closed
ggetz opened this issue Feb 19, 2018 · 8 comments
Closed

CesiumMath vs Math naming ambiguity #6233

ggetz opened this issue Feb 19, 2018 · 8 comments

Comments

@ggetz
Copy link
Contributor

ggetz commented Feb 19, 2018

It's not clear that the CesiumMath class is included in the namespace as Cesium.Math. This is also inconsistent with other classes that have the Cesium prefix, like Cesium3DTileset. If this is not something we want to change in the API, this should be made clear in the documentation.

Relevant forum thread: https://groups.google.com/forum/#!topic/cesium-dev/icpMxc_bea8

@hpinkos
Copy link
Contributor

hpinkos commented Feb 19, 2018

CesiumMath isn't a class. The class is Math, which is why it's Cesium.Math.

We use CesiumMath for the name when we're requireing Math.js via AMD so we don't overwrite the generic JavaScript math functions.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 19, 2018

We should probably rename the class to CesiumMath so it matches the documentation

@mramato
Copy link
Contributor

mramato commented Feb 19, 2018

I'm not crazy about renaming the class to CesiumMath, I hate the Cesium prefix that is starting to appear everywhere (Cesium3DTileset, CesiumIon, etc.. ) because a lot of user code ends up with Cesium.CesiumXXX and it gets really verbose.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 19, 2018

@mramato The problem we need to fix is that documentation says CesiumMath because that's what the name of the variable is in Math.js and that's what we export.

https://cesiumjs.org/Cesium/Build/Documentation/CesiumMath.html

Is there a way to just fix the documentation to say Math?

@mramato
Copy link
Contributor

mramato commented Feb 19, 2018

Changing the exports in Math.js would do it, the only downsize is that it will then be "wrong" for module users. Do maybe you guys are right and CesiumMath is the better choice.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 19, 2018

If most developers use Cesium.js, let's please make the Cesium. name most natural and just "fix" the doc even if it means adding a custom tag.

Cesium3DTileset and CesiumIon are both special cases - can't start a variable name with a number, and a product name.

If this is not urgent, I don't know that we need to burn too many cycles on it now.

@mramato
Copy link
Contributor

mramato commented Feb 19, 2018

It should literally be a one line change to update the doc.

@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/icpMxc_bea8

If this issue affects any of these threads, please post a comment like the following:

The issue at #6233 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.


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

🌍 🌎 🌏

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

No branches or pull requests

5 participants