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

Replace lazy_static with OnceLock #160

Closed
wants to merge 3 commits into from
Closed

Replace lazy_static with OnceLock #160

wants to merge 3 commits into from

Conversation

russellbanks
Copy link
Contributor

@russellbanks russellbanks commented Jan 1, 2024

This pull request removes lazy_static in favour of OnceLock. OnceCell is technically a crate. However, it has been partially merged into the standard library.

lazy_static hasn't had a release in 4 years and now has a section on its ReadMe that shows how to achieve the exact same thing with the standard library.

Copy link
Collaborator

@brotskydotcom brotskydotcom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good, but I don't want to merge this yet. See other comments in the PR.

@brotskydotcom
Copy link
Collaborator

brotskydotcom commented Jan 2, 2024

I don't want to move the minimum build platform for this crate to 1.70 (June 2023), which is required by these changes. There are a lot of clients on second-tier platforms (such as the BSDs), and I have seen the built-in rust on those platforms be a year or more old. I released v2 in Feb 2023 (on Rust 1.68), and I'd rather not break backward compatibility by moving the library to require 1.70. (Yes, the example uses clap 4.22, which requires Rust 1.70, but folks who use the library don't need clap.)

I'm happy to revisit this when the 2024 edition ships, and I expect to release a v3 somewhere around then that will integrate this change. Marking this PR as waiting until then.

@brotskydotcom brotskydotcom added the waiting Waiting for an action outside the repo label Jan 2, 2024
@brotskydotcom brotskydotcom self-assigned this Jan 2, 2024
@russellbanks
Copy link
Contributor Author

russellbanks commented Jan 2, 2024

Until then, we could use the actual OnceCell crate? It has an MSRV of 1.60. We'd still benefit from it being maintained and it achieves the same thing as lazy_static without using a macro.

@russellbanks russellbanks changed the title Replace lazy_static with OnceLock Replace lazy_static with once_cell Jan 2, 2024
@russellbanks
Copy link
Contributor Author

I've replaced this with Lazy from OnceCell which more closely matches lazy_static. Once LazyCell (standard library equivalent of Lazy) gets stabilised and/or we move to a higher MSRV, we can replace this with OnceLock or LazyCell.

Copy link
Collaborator

@brotskydotcom brotskydotcom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look fine. Again, see comments further down.

@brotskydotcom
Copy link
Collaborator

brotskydotcom commented Jan 2, 2024

How about, instead of just switching crates from lazy_static to OnceCell, we instead leave lazy_static in place for older compilers, add the rustversion crate, and use the stable language feature you were originally suggesting on MSRV of 1.70 or higher? Since almost everyone is on 1.70 anyway, I'd rather make a change to the stable language feature now instead of just picking up a different crate dependency that affects everyone (and that i would have to carefully test on 1.68 to be sure there's no change).

@russellbanks russellbanks changed the title Replace lazy_static with once_cell Replace lazy_static with OnceLock Feb 28, 2024
@brotskydotcom
Copy link
Collaborator

I took these changes as part of #179. Thanks so much for your contribution!

@brotskydotcom brotskydotcom removed the waiting Waiting for an action outside the repo label Jul 6, 2024
@russellbanks russellbanks deleted the once_cell branch July 11, 2024 19:10
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

Successfully merging this pull request may close these issues.

2 participants