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

Supply a Preconditions in common location #800

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Conversation

pisv
Copy link
Contributor

@pisv pisv commented Feb 13, 2024

This is a follow-up to #798, which handles Preconditions similarly to ToStringBuilder.

See #742 for detailed discussion.

@pisv
Copy link
Contributor Author

pisv commented Feb 13, 2024

Note that #798 needs to be merged first.

@jonahgraham
Copy link
Contributor

I had another PR almost ready to go on this topic, but I didn't get it up before the end of the day.

I was a little more weary of removing Preconditions because it is referenced from a number of places: https://github.com/search?q=Preconditions.enableNullChecks&type=code

Do you think it is ok to have the breaking change that says means consumers need to change their imports? I assume that no one is really depending on the sort of scoping that the enableNullChecks provides by having multiple copies.

@jonahgraham
Copy link
Contributor

The documentation added in #799 needs to be updated too.

@jonahgraham
Copy link
Contributor

My alternate is in #801

This is a follow-up to #798, which handles `Preconditions` similarly to
`ToStringBuilder`.

See #742 for detailed discussion.
@jonahgraham
Copy link
Contributor

After more thought I want to go with this version and since there are already breaking changes in 0.22.0 compared to 0.21.0 I think we should remove rather than just deprecate the old Preconditions.

@pisv before I rebase and merge this I will wait a little while for your comment. I am trying to complete the 0.22.0 today if possible.

@pisv
Copy link
Contributor Author

pisv commented Feb 13, 2024

After more thought I want to go with this version and since there are already breaking changes in 0.22.0 compared to 0.21.0 I think we should remove rather than just deprecate the old Preconditions.

Makes sense to me, although both alternatives have their merits...

@jonahgraham
Copy link
Contributor

I have rebased and pushed. On successful build I will merge this.

@jonahgraham jonahgraham added this to the 0.22.0 milestone Feb 13, 2024
@jonahgraham jonahgraham merged commit 3002aa3 into main Feb 13, 2024
7 checks passed
@jonahgraham jonahgraham deleted the pisv/preconditions branch February 13, 2024 15:01
@pisv
Copy link
Contributor Author

pisv commented Feb 13, 2024

The documentation added in #799 needs to be updated too.

Looks like I have completely missed that part, sorry... Unfortunately, I'll not be able to do it right now.

@jonahgraham
Copy link
Contributor

No worries. I forgot to do it too. I miss Gerrit, it warns me before merging if there are unresolved comments.

I have the update on my other pr and will provide it soon.

@jonahgraham
Copy link
Contributor

I have the update on my other pr and will provide it soon.

Done in #804

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

2 participants