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

Resolve inconsistency between getPropertyIds and getPropertyNames #9914

Closed
sanjeetsuhag opened this issue Nov 4, 2021 · 2 comments · Fixed by #10473
Closed

Resolve inconsistency between getPropertyIds and getPropertyNames #9914

sanjeetsuhag opened this issue Nov 4, 2021 · 2 comments · Fixed by #10473
Assignees

Comments

@sanjeetsuhag
Copy link
Contributor

sanjeetsuhag commented Nov 4, 2021

For 3DTILES_metadata, classes like TilesetMetadata, TileMetadata and GroupMetadata have a property called getPropertyIds(), whereas, ModelFeature and Cesium3DTileFeature have getPropertyNames. These should ideally all have the same signature.

We should go with getPropertyIds.

@lilleyse
Copy link
Contributor

Since getPropertyNames is part of the public API lets deprecate it in the upcoming release (CesiumJS 1.95) and remove it three months from now (CesiumJS 1.98).

For the PR:

  • Add getPropertyIds wherever getPropertyNames appears. Update any code and sandcastles to use getPropertyIds
  • Mark getPropertyNames as deprecated. See Model.js#L708-L730 for an example.
  • Note the deprecation in CHANGES.md under the CesiumJS 1.95 release.

After the PR is merged, open an issue to remove getPropertyNames and assign the label remove in 1.98 (you may have to create the label). See #10416 for an example.

@javagl
Copy link
Contributor

javagl commented Jun 22, 2022

I actually stumbled over this when creating the sample sandcastles (and the fact that // It's all just metadata, eventually wasn't resolved by the fact that I used an obscure approach to obtain what I then called "property keys", once being IDs and once being names). (I'll update thes latest versions of these sandcastles when this is merged).

The more technical question is whether this leads to the respective classes actually being what is referred to as an 'implementation of an interface' in the Java world, with the interface being MetadataEntity here. The name MetadataEntity does not appear publicly yet. I wondered whether the goal was to be able to say: "Whatever you have there, if it is 'metadata', then you can call these functions on it". But maybe that's beyond the scope of this issue and the PR.

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

Successfully merging a pull request may close this issue.

5 participants