Helper methods to modify leading and trailing trivia on nodes #91
Conversation
| } | ||
| set { | ||
| if let trailingTrivia = newValue { | ||
| self = withTrailingTrivia(trailingTrivia) |
There was a problem hiding this comment.
What if I set this to nil?
There was a problem hiding this comment.
This is a tricky one. As the getter returns nil when there is no token in the hierarchy, it doesn't make sense to set nil: it's not like it should remove all tokens form the hierarchy. The only options I thought of are:
- Have the setter do nothing when setting nil (that's the option I chose). Unfortunately, this can be very surprising:
print(node.leadingTrivia) // [.spaces(1)]
node.leadingTrivia = nil
print(node.leadingTrivia) // [.spaces(1)]- Have the setter trap when setting nil. That's another good option AFAIK.
- Don't implement a setter and only rely on
with...Triviafunctions. I'm not a fan of this option as we have convenience setter for all other properties.
There was a problem hiding this comment.
What about making node.leadingTrivia = nil have the same semantics as setting node.leadingTrivia = []? Without looking too much into detail, it's a behaviour I would expect / would be able to explain when it occurs.
There was a problem hiding this comment.
That's indeed another option, but it still has the same surprising behaviour as noop that the getter won't be returning the same value you just set.
There was a problem hiding this comment.
Yeah, still both values represent some kind of “nothingness”.
It also reminded me of some Cocoa pattern where setting a property to nil caused it to take on a particular value (I think like black for text colour). It's not really relevant here but it shows that having the setter set a different value than is afterwards returned by the getter is not completely unprecedented in the Apple ecosystem.
There was a problem hiding this comment.
Sounds fair, I'll make the change.
There was a problem hiding this comment.
FWIW that pattern is codified into the language as null_resettable. I think it's perfectly valid here.
hartbit
force-pushed
the
syntax-trivia
branch
from
dc805f7 to
c0266e1
Compare
|
@ahoppen @nkcsgexi @harlanhaskins Any news? |
|
@swift-ci please test |
|
Sorry for the delay here, @hartbit! |
|
Thanks!! |
Fix scrunching of range operators.
This is implemented recursively on
RawSyntaxand throughSyntaxDataand appropriate public methods were made available on all syntax nodes and collections.