+1, the proposal provides sweet little utility with unambiguous name that cuts boilerplate in half of more (see Optionals chaining examples above). That is always a win in my book.
What is your evaluation of the proposal?
Mildly +1.
The only reason I say mild is I can see the potential for this to be misused by devs who toggle and assume their state is consistent with the reason they’re toggling it eg from a UI component. I would recommend devs be explicit where possible rather than use functionality like this and risk inconsistent state propagating. I can definitely see the use case, though.
Is the problem being addressed significant enough to warrant a change to Swift?
It can be tough to deal with deeply nested Booleans with optional chaining without duplicate boilerplate. I’m not 100% sure that something this convenient is wise, though...
Does this proposal fit well with the feel and direction of Swift?
It’s definitely a functional and useful tweak. Though we did evaluate an array[ifExists: index] bounds checked convenience to be a bad idea, if I recall...
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
N/A
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Quick reading.
+1 to .flip() from me. "Toggle" as a verb sounds a bit Americanized. "Flip" seems more vivid.
+1 from me.
I mildly prefer the name .invert(), as it's less skeuomorphic.
2 Likes
Agree. I was even thinking that .switch() would be more universally understood as it relates to programing.
I'm in favor of this change.
I don't think comparing toggle() to ++ is valid for the same reasons Chris mentioned.The name toggle is my preference. flip sounds too bit-wisey to me. invert is ok but less clear than toggle. switch already means something else in the language and should not be overloaded in my opinion.
2 Likes
I'm generally in favour of the proposal because of the 'argument from random usefulness' and the 'argument from optional chaining'.
I'm wondering out loud here: could this proposal encourage developers to write less meaningful code?
For example, if I had a LightControl type, I wouldn't expose the boolean property modelling the state because it is more meaningful in the model's domain to say:
var control = LightControl()
control.switch()
print("the light is \(control.isOn ? "On" : "Off" )")
...rather than:
var control = LightControl()
control.isOn.toggle()
print("the light is \(control.isOn ? "On" : "Off" )")
The difference seems pedantic, which makes me inclined to think there's less potential harm to code quality and readability. But is that accountable to the quality of the example or the invalidity of the concern for code quality?
It's also hard to think of other occasions where you want to toggle a boolean value rather than setting booleans to a particular state in bulk (eg: mark as unread/read).
Maybe someone smarter than me can point out the error of my concerns?
2 Likes
+0.1, though I’d prefer toggled for a non-mutating and toggle for mutating.
2 Likes
Yes! Thank you; that is more or less what I have been trying to say. For a light it makes more sense for the programmer to decide on the best name of the method. For a light .switch makes more sense. For something like .isMovingFoward then .revert(); For .isUpside then perhaps .invert() makes more sense for my application, etc, etc.
This sounds like an argument for providing no functionality at all in the standard library, because it might be used in a context where the name isn't optimal. This is a method on Bool, not on LightControl, so the name only has to make sense for Bool, not for any type that may use a Bool internally. You can, of course, implement a method on LightControl, with whatever name you prefer, and then call toggle() on the Bool representing its internal state.
3 Likes
+1, with notes below:
I don't think the proposal lays out a convincing argument that it should be included, and it provides very little in terms of use cases that would be significantly improved.
However, this post: SE-0199: Adding toggle method to Bool - #16 by Ben_Cohen does a great job in laying out some of the benefits.
My personal thoughts on the matter: I really enjoy when standard libraries do things that help in the pragmatic case. I think this is one of those.
3 Likes
+1
I was of the opinion that this wasn't a useful enough addition to add to the standard library. It certainly wan't harmful, but @Ben_Cohen's response with the examples of its usage has swayed me to be in favor of it.
I would prefer .toggled() be added as a non-mutating version as well though and it seems like a minor and harmless thing to add to this proposal.
I also agree that the name should be toggle, of course, I am an American so that might be why I like it ;)
+1
I think this solves a real problem, and is a welcome addition. There's precedence for methods like this being included in the standard library (see: Int.negate), so this seems perfectly reasonable.
2 Likes
My mistake, I meant .negate() -- corrected!
I'm not sure what the point of having the toggled() non-mutating version would be. There is already an operator for this, and it wouldn't have the same advantage of reducing repetition on both sides of an assignment.
I write someLong.chained.booleanExpression() == false all the time because for long expressions, the ! at the beginning can get lost easily. toggled() would "solve" that for me
To clarify my review manager note:
toggled() is a reimplementation of ! as a function, and as such is probably on the commonly rejected proposals list. I say probably because the reasoning for rejecting that particular variant isn't listed, but I expect we'd consider it covered by it.
Whether to add toggled() or not would also be a separate proposal that doesn't affect whether or not a mutating toggle() should be accepted, so if it weren't on the commonly-rejected list, it should be raised as a new pitch rather than discussed in this review.
I’m in favour of the concept but I frankly do not feel strongly either way on its inclusion, it blurs the line between being moderately useful and being too trivial to warrant its existence.
The proposed name toggle() does not feel appropriate. I think it implies that something else is happening, as if it’s a higher-level abstraction of some kind. In a GUI app context, the signature could get confused for a UI component action for example. Standard library API should be blandly named, toggle() isn’t ‘boring’ enough. invert() is my recommendation.
5 Likes
Great big +1 from me--even if for just this example. It would really improve composition with optional chaining. Yes, I could just implement this myself, but it would be nice to not have to do so for each project (personally I could get plenty of good use out of this). Not terrible if it doesn't make it in though.