-
Notifications
You must be signed in to change notification settings - Fork 228
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
Fix formatting issue with IfConfigDecl/ImportDecl interaction. #860
base: main
Are you sure you want to change the base?
Conversation
@allevato I'm wondering if you have any thoughts on this; I want to make sure I'm understanding everything correctly and that there aren't any other logic bugs. We insert a |
I think we want to keep the overall stylistic idea that "imports look better all on one line", and so we only need to fix that logic specifically for imports that have So, I think we can address this a different way. What if, in Sure, that breaks the promise we're making to simpler tools that might want to quickly scan for those imports, but they were never going to handle something that complicated anyway without a full parse (and |
That sounds like a more promising approach than what I'd considered so far, so I'll give that a try. Thanks for your thoughts! |
a1fd498
to
2065608
Compare
The ImportDeclSyntax attributes might include IfConfigDecls, which absolutely require line breaks in certain positions. The disable breaking logic of ImportDecl was preventing these from being emitted.
2065608
to
4f4be5d
Compare
@@ -510,7 +510,7 @@ final class IfConfigTests: PrettyPrintTestCase { | |||
#if os(iOS) | |||
.iOSSpecificModifier() | |||
#endif | |||
.commonModifier() | |||
.commonModifier() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these changes be avoided? The idea is that the indentation of these would be the same if there weren't an intervening #if
-config; i.e., we wouldn't have this:
SomeFunction(
foo,
bar
)
.commonModifier()
The ImportDecl syntax node disables all line breaks (other than
hard
). This creates code that fails to compile when#if
/#endif
are involved (see example), removing all the line breaks after the#if <clause>
as well as the#endif
. Switching to hard line breaks would certainly work but feels a bit heavy handed and could end up with extra line breaks in other scenarios.