Skip to content

Support @Nullable reasons in ConditionEvaluationResult APIs #4699

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbrannen
Copy link
Member

@sbrannen sbrannen commented Jun 30, 2025

Overview

This PR addresses the issues raised in #4698 by annotating all "reason" parameters with @Nullable.

Related Issues

@sbrannen
Copy link
Member Author

sbrannen commented Jun 30, 2025

However, while working on this issue I realized two things.

  1. We always allowed null for any of the reasons, but the Javadoc did not reflect that.

  2. When a custom reason was supplied but a null was supplied for the default reason to ConditionEvaluationResult.disabled(String, String), the generated reason was "null ==> custom"; whereas, ideally that should just be "custom".

  3. We allowed a blank reason (such as an empty string or a string containing only whitespace).

We should decide if we want to backport any of the following to 5.13.x.

  • the "null is allowed" Javadoc changes
  • the "null ==> custom" bug
  • the conversion of an empty/blank reason string to an empty Optional

@sbrannen sbrannen force-pushed the issues/4698-ConditionEvaluationResult-nullability branch from 8062b9f to ae16e45 Compare June 30, 2025 12:31
@marcphilipp
Copy link
Member

We should decide if we want to backport any of the following to 5.13.x.

  • the "null is allowed" Javadoc changes
  • the "null ==> custom" bug
  • the conversion of an empty/blank reason string to an empty Optional

I think we should backport all of them.

@sbrannen
Copy link
Member Author

I think we should backport all of them.

In that case, I will create a separate GitHub issue to track all of that, since they are actually unrelated to the @Nullable changes necessary on main for 6.0.

@sbrannen sbrannen force-pushed the issues/4698-ConditionEvaluationResult-nullability branch from b9b3c7e to b206754 Compare June 30, 2025 14:52
sbrannen added a commit that referenced this pull request Jun 30, 2025
Prior to this commit, we did not have any "unit tests" for
ConditionEvaluationResult, and while working on #4698 I noticed that we
in fact have several issues in the implementation of and documentation
for ConditionEvaluationResult.

Thus, this commit introduces dedicated unit tests for the status quo,
where individual TODOs will be addressed in separate issues/commits.

See #4698
See #4699 (comment)
marcphilipp pushed a commit that referenced this pull request Jul 1, 2025
Prior to this commit, we did not have any "unit tests" for
ConditionEvaluationResult, and while working on #4698 I noticed that we
in fact have several issues in the implementation of and documentation
for ConditionEvaluationResult.

Thus, this commit introduces dedicated unit tests for the status quo,
where individual TODOs will be addressed in separate issues/commits.

See #4698
See #4699 (comment)

(cherry picked from commit 3026c62)
sbrannen added a commit that referenced this pull request Jul 2, 2025
sbrannen added a commit that referenced this pull request Jul 2, 2025
See #4698
See #4699

(cherry picked from commit 6b7f17d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants