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 once_cell #133

Closed
wants to merge 5 commits into from
Closed

Replace lazy_static with once_cell #133

wants to merge 5 commits into from

Conversation

Lonami
Copy link

@Lonami Lonami commented May 23, 2023

This should make it easier for the ecosystem to trim down the dependency tree.

I didn't update rspec to the 1.0 version because it seemed to pull a bunch other things and is only used in dev mode.

I think this is technically a breaking change, so let me know if we should bump the version number to 0.4.

@bb010g
Copy link

bb010g commented May 23, 2023

As long as SHOULD_COLORIZE is exposed to other crates, this is a breaking change.

@mackwic
Copy link
Collaborator

mackwic commented Jul 2, 2023

Hi I don't get why we should remove lazy_static, would you develop the reasoning behind this PR?

Without good reason, I would prefer to do nothing and close this PR.

Copy link
Collaborator

@mackwic mackwic left a comment

Choose a reason for hiding this comment

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

Please explain why we should do this change

@hwittenborn
Copy link
Member

I do think once_cell has a cleaner API @mackwic (albeit not from the perspective of consumers of the library), but it probably would be a breaking change for any consumers of the API.

The only logic for it being a breaking change (from what I've seen) is that the static implements LazyStatic, but I'm not sure how big of a deal that is for you. Assuming this were to go through I'd probably make it a breaking change and likewise a major versoin bump (which I'd also prefer to avoid), but I'm fine with whatever. Just throwing it out as some thoughts.

@Lonami
Copy link
Author

Lonami commented Jul 2, 2023

The ecosystem has been migrating from lazy_static to once_cell. once_cell has been added to Rust's std, which means that for newer versions of the compiler, the once_cell crate can compile to a mere re-export of std. But it's still useful to depend on it if we want to provide support for lower MSRV.

My main motivation as hinted in the PR description is keeping the dependency tree small where possible.

I don't know how important the SHOULD_COLORIZE constant really is. To be honest, I would personally replace it with a fn.

@hwittenborn
Copy link
Member

Sorry to just now mention this @Lonami, but it looks like #119 is doing the same thing, and I'd probably accept that one just because the PR was made first.

If you'd want to make a PR for that dev_dependencies thing in Cargo.toml I'd be fine getting that merged in. Really sorry to not get this one in though.

@hwittenborn hwittenborn closed this Jul 3, 2023
@hwittenborn
Copy link
Member

If you think this one merits enough to get merged in still I'd be fine reopening it though, just let me know.

@Lonami
Copy link
Author

Lonami commented Jul 3, 2023

I was unaware the other PR existed. I had trouble running cargo when the TOML section was called dev_dependencies (underscore as opposed to dash). I also no longer see a need for the extern crate once_cell from the PR.

But ultimately I don't really care how it's done, as long as it gets merged eventually. I'll subscribe to the other PR instead. Thanks!

@hwittenborn
Copy link
Member

Yeah I'm not sure why the section in Cargo.toml was called dev_dependencies instead of dev-dependencies, I'm assuming it was from something an old version of Cargo handles things, but I'm not really sure honestly.

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.

None yet

4 participants