georust / geographiclib-rs

A port of geographiclib in Rust.
MIT License
40 stars 9 forks source link

Make lazy_static optional? #46

Closed Quba1 closed 9 months ago

Quba1 commented 2 years ago

lazy_static is used once in the whole crate - to declare wgs84 ellipsoid. So lazy_static is not necessary for the main functionality of the crate, but is introduced as a compulsory dependency.

Having a reference ellipsoid in the crate is definitely a useful feature to not require from user to define WGS84 manually every time (and it's also useful for tests). But the introduction of a additional dependency for one object seems like a good spot for improvements.

My idea is to add pre-defined ellipsoids (and thus lazy_static) as a default feature. This way no functionality will change for users, but they will be able to use one less dependency when needed.

I can create a PR if such solution is welcome.

michaelkirk commented 2 years ago

It's nice to trim down dependencies, but I'm wondering how many people actually want to use this library without the WGS84 ellipsoid. Do you have this use case? Can you tell me more about it?

lazy_static is a very small, common, and well maintained dependency, so I'd guess that it's unlikely to cause any real damage.

More likely, in my estimation, is that some well meaning person will disable all the default features on their dependencies and then wonder why this library no longer works for them.

Quba1 commented 2 years ago

Do you have this use case? Can you tell me more about it?

I have this exact case where lazy_static seems like suboptimal addition. I'm using geographiclib only in one part of my library, and thus I'm implementing my own Ellipsoid (much leaner) type which implements From/Into Geodesic. So I'm not using the internal geographiclib's wgs84 except for tests.

lazy_static is a very small, common, and well maintained dependency, so I'd guess that it's unlikely to cause any real damage.

That's why I'm not strongly arguing for implementing this change, but rather discussing it as something to consider.

More likely, in my estimation, is that some well meaning person will disable all the default features on their dependencies and then wonder why this library no longer works for them.

Personally, I would assume that mostly experienced Rust users disable default features and expect things to break because of that, and look for the smallest set of necessary features for their use-case.

Dushistov commented 2 years ago

My idea is to add pre-defined ellipsoids (and thus lazy_static) as a default feature. This way no functionality will change for users, but they will be able to use one less dependency when needed.

It is possible just pre-calculate WGS84_GEOD:

static WGS84_GEOD: Geodesic = Geodesic {
   a: WGS84_A
...
};

and add unit test to verify that it equals to Geodesic::new(WGS84_A, WGS84_F) to catch need of change pre-calculate code if Geodesic::new would modified.

So it would be possible unconditionally remove lazy_statics, and also make code little faster because of there is no need for read/write atomic in every access.

frewsxcv commented 10 months ago

It should be possible to replace the lazy_static usage with std::sync::OnceLock

michaelkirk commented 9 months ago

Fixed in #52