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

[API-8] Improve Tristate #2137

Merged
merged 6 commits into from
Jul 2, 2020
Merged

[API-8] Improve Tristate #2137

merged 6 commits into from
Jul 2, 2020

Conversation

ItsDoot
Copy link
Member

@ItsDoot ItsDoot commented Jun 30, 2020

Adds:

  • Tristate#not
  • Tristate#fromNullableBoolean
  • Tristate#asNullableBoolean

* Add Tristate#not
* Add Tristate#fromNullable
@@ -94,6 +126,13 @@ public static Tristate fromBoolean(boolean val) {
*/
public abstract Tristate or(Tristate other);

/**
* NOTs this tristate.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call it negate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Edited

Copy link
Member

Choose a reason for hiding this comment

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

Maybe also change the method name accordingly?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO not lines up better with and/or than negate does.

Copy link
Contributor

@dualspiral dualspiral left a comment

Choose a reason for hiding this comment

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

Looks okay, I think a method needs renaming and one should be added for consistency.

* @param val The nullable boolean value
* @return The appropriate tristate
*/
public static Tristate fromNullable(@Nullable Boolean val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be named fromNullableBoolean.

If we have this, should we also have an asNullableBoolean analogue?

Copy link
Member Author

@ItsDoot ItsDoot Jun 30, 2020

Choose a reason for hiding this comment

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

See, I was thinking about doing asNullableBoolean, but what should it return? Returning an actual nullable boolean would make sense, but Sponge prefers Optionals... which to me would feel a little weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of those rare cases I'd have a nullable return type as a one-to-one mapping of Boolean<->Tristate - if you can get a Tristate from a Boolean, it stands to reason that you should be able to go the other way just as simply.

I'll let @Zidane call this one, but I think it should just be a @Nullable Boolean return type. There is an extra clue in the name of the method here, if you're not expecting the possibility of it being null if you grab the Boolean then that's really your lookout.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer, not to have a asNullableBoolean, because this class is the replacement for the nullable/unset boolean.

As for the fromX methods, I would rather name them both from (or fromBoolean) with just different parameters. Let the compiler decide which one it should use.

Copy link
Contributor

Choose a reason for hiding this comment

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

because this class is the replacement for the nullable/unset boolean.

That doesn't change my argument at all. I'm saying the mapping should work both ways in case you then interact with some system that isn't Tristate aware - plugins can (and often do) work with third party libraries.

Copy link
Member Author

@ItsDoot ItsDoot Jun 30, 2020

Choose a reason for hiding this comment

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

Changed fromNullable to fromNullableBoolean, and added asNullableBoolean.

As for the fromX methods, I would rather name them both from (or fromBoolean) with just different parameters. Let the compiler decide which one it should use.

@ST-DDT I'd rather users explicitly opt-in to using the nullable boolean conversions. Then it's clear when someone else is reading your code, for example.

@ST-DDT
Copy link
Member

ST-DDT commented Jun 30, 2020

While we are at it we should also change:

TRUE(true) {
@Override
public Tristate and(Tristate other) {
return other == TRUE || other == UNDEFINED ? TRUE : FALSE;
}

to

return other == FALSE ? FALSE : TRUE;

This is shorter/easier to understand and also matches the FALSE#or(...) style.

@ItsDoot
Copy link
Member Author

ItsDoot commented Jun 30, 2020

Agreed @ST-DDT

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.

4 participants