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

Make lazy_static optional? #46

Closed
Quba1 opened this issue Aug 16, 2022 · 5 comments
Closed

Make lazy_static optional? #46

Quba1 opened this issue Aug 16, 2022 · 5 comments

Comments

@Quba1
Copy link
Contributor

Quba1 commented Aug 16, 2022

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
Copy link
Member

michaelkirk commented Aug 17, 2022

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
Copy link
Contributor Author

Quba1 commented Aug 20, 2022

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
Copy link

Dushistov commented Aug 27, 2022

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
Copy link
Member

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

@michaelkirk
Copy link
Member

Fixed in #52

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

No branches or pull requests

4 participants