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

[scala2] feat: adding on-demand default evaluation #533

Merged
merged 8 commits into from
May 23, 2024

Conversation

Andrapyre
Copy link
Contributor

@Andrapyre Andrapyre commented May 20, 2024

Description

Currently, all default values are precompiled. This has led to random generators, such as random UUID.randomUUID() and Instant.now() being memoized (see zio/zio-json#1055). This PR adds an option to evaluate default parameters on-demand. See the accompanying PR for Scala 3 support: #534

@Andrapyre Andrapyre changed the title feat: adding on-demand default evaluation [scala2] feat: adding on-demand default evaluation May 21, 2024
@adamw
Copy link
Member

adamw commented May 21, 2024

Thanks for the PR! I'll take a detailed look in a couple of days, but for sure we'll need to find a way to keep bincompat. Or move these changes to v2.

@joroKr21
Copy link
Contributor

Since this would break new CallByNeed(param) and force users to use CallByNeed(param), I tried to find some examples of libs where users would need to make this change, but I couldn't find any instances outside of Magnolia where this implementation is actually being used. See this search. It seems that breaking this would likely have minimal effect, if any.

It can break when you have two dependencies that use Magnolia on the classpath, one the older version using the constructor, and another one the newer version using apply. You can add another constructor to keep binary compatibility.

@Andrapyre
Copy link
Contributor Author

I've adjusted both PRs to keep bincompat. Build should be passing now.

@adamw adamw merged commit a633965 into softwaremill:scala2 May 23, 2024
4 checks passed
@adamw
Copy link
Member

adamw commented May 23, 2024

Thanks for the PR!

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.

3 participants