Fix more postfix pound if scenarios #402
Conversation
| var this: Syntax? = Syntax(node) | ||
|
|
||
| while this?.parent != nil { | ||
| if this?.is(TupleExprElementSyntax.self) == true { |
There was a problem hiding this comment.
This handles the scenario in this test
There was a problem hiding this comment.
This is the fragile part which I dislike. It handles the scenario in that test fine, but what other scenarios are there that this does not handle, or that this breaks?
This did not pan out. My thought was "why don't we just look for the right ancestor instead of the postfix if config ancestor?" But from what I can tell, the tree for a postfix if config looks something like this:
Given this example: EmptyView()
.padding([.vertical])
#if os(iOS)
.iOSSpecificModifier()
#endif
I simplified this so we don't have to go back down the syntax tree.. |
| #if os(iOS) | ||
| .iOSSpecificModifier() | ||
| #endif | ||
| .commonModifier() |
There was a problem hiding this comment.
This is not handled properly by these changes because I didn't think to try adding this until I was creating this pull request. Will look into how to solve this but wanted to have it in this pull request so I didn't forget to mention it.
There was a problem hiding this comment.
Have you had any luck with this one? For consistency we'd definitely like if the #if aligns with the preceding . and then the contents are indented one more level.
There was a problem hiding this comment.
I have not yet, things at work got really busy soon after I created this. I'll take another look at it soon.
There was a problem hiding this comment.
Took me a bit longer than I expected to get back to this. I think this latest commit works as intended. Let me know if I misunderstood.
I don't love the implementation I landed on so I'l be looking to improve it if I can.
There was a problem hiding this comment.
I'm happier where this is now. Would appreciate any and all feedback :)
There was a problem hiding this comment.
@allevato Friendly ping for awareness, not urgency, here since it took me so long to get back to this. I think it is behaving how you described now.
There was a problem hiding this comment.
Sorry for the delay; I lost track of this one. These results look great, thanks for getting all these cases working so well!
I had this bit a little wrong, I believe this is correct:
Based upon this, I was able to simplify the implementation. |
|
This should probably be two different pull requests, I'm happy to break them up if others agree. |
| #if os(iOS) | ||
| .iOSSpecificModifier() | ||
| #endif | ||
| .commonModifier() |
There was a problem hiding this comment.
Sorry for the delay; I lost track of this one. These results look great, thanks for getting all these cases working so well!
Fix more postfix pound if scenarios
Demonstrates the issue in #392 and provides a potential fix. The fix feels fragile but from what I understand from the syntax tree I'm not sure how to make it less fragile.
My previous pull request only looked up the tree to see if an ancestor was a postfix pound if but this is not sufficient to format properly because a child node having a postfix pound if ancestor does not always mean that that child is nested inside of the postfix pound if. This change traverses back down the tree to ensure that the child node is nested properly.
While typing that out I might have had an epiphany about a better way to solve this, if so I'll update. Either way I'd like to get the test scenarios out there to document those failures.