BREAKING CHANGE: add non-geospatial option

[jeffp123]

📇 Description

This adds a new non-geospatial option.

I still have yet to update the comments and document and add more test cases. I can add those things if there is any interest in this PR.

For now, I mostly wanted to confirm that it can be done.

🚨 Checklist

• My changes do not introduce backwards compatibility issues.
• I have commented my code, particularly in hard-to-understand areas.
• I have actively considered and searched for potential security issues and did address these.

🧪 Tests

• I have added missing tests for related code.
• I have manually tested my code for regression on existing functionality.
• I have added tests that prove my fix is effective or that my feature works.

[slavik-pastushenko]

Thanks for contributing!

I left a few comments. One more thing, can you update the README.md file by updating the Options and Usage sections, please?

[slavik-pastushenko]

please use . in the end instead of ?.

[slavik-pastushenko]

and maybe describe what it means so it is easier for everyone to understand.

[slavik-pastushenko]

I don't think we need a new method that is exactly the same except for the use_non_geospatial_coords property value. We can reuse the existing new by setting the cluster_type enum property from the options parameter.

[jeffp123]

Can you advise how to do this? I am fairly new to Rust, and I cannot think of a way to add a property to Options without breaking backwards compatibility.

[slavik-pastushenko]

Sure,

• add a new boolean flag here to be able to manage it when setting a new cluster: https://github.com/chargetrip/supercluster-rs/blob/main/src/lib.rs#L27.
• add a boolean flag here, to be able to access it through the cluster itself: https://github.com/chargetrip/supercluster-rs/blob/main/src/lib.rs#L49.
• after that, you will have a property value to set here during the initialisation process: https://github.com/chargetrip/supercluster-rs/blob/main/src/lib.rs#L87.

[jeffp123]

That does not sound right. If let's say I add a new parameter Options is_non_geospatial: bool, then any place that I set the Options that are passed to Supercluster will need to include is_non_geospatial in those options.

[slavik-pastushenko]

What about using an enum instead of a boolean flag? This way it is clearer which cluster type we want to use. Check the comments above about this suggestion.

[slavik-pastushenko]

Indeed, we can use an enum type to control it instead of a boolean flag.
The only place we need options is when we initialise Supercluster.

[jeffp123]

Ok. So, I add:

cluster_type: ClusterType

to Options.

Now any previous code that instantiates a new Supercluster that wants to use the default geo-spatial use-case will break because cluster_type is missing.

[slavik-pastushenko]

Correct, we introduce a breaking change but I will mention it in the upcoming Changelog.md.

[jeffp123]

Ok, as long as you are cool with it, I like this a lot more.

I was mostly trying to protect against a breaking change.

In other languages (i.e. Python), I would just introduce the new option with a default value. I wish there was some way to do something similar in Rust. Alas, Rust demands being explicit. Not a bad thing, but just different.

Ok, I will introduce the breaking change as suggested!

[slavik-pastushenko]

thanks!
I will update CI to generate release notes and it will be visible that we generated a breaking change.

[slavik-pastushenko]

Please update the description of the get_cluster_json for this property.

[slavik-pastushenko]

it can be an enum value from cluster_type instead of a boolean flag.

[slavik-pastushenko]

What about replacing it with an enum:

pub enum ClusterType {
 GeoSpatial,
 
 NonGeoSpatial,
}

and use it instead of a boolean flag. This way it will be easier to understand and maintain.