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

Fix formatting of import with multiple attributes (fixes #445) and ensure that imports never wrap #501

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

stackotter
Copy link
Contributor

#445 occurs because Swift Format simply just adds a space after the attribute list of import statements. arrangeAttributeList can be reused to correctly format the attributes of an import. However, when modifying the test for imports to include an import decl with multiple attributes, I found that attributes with arguments (e.g. @_spi(STP)) get wrapped when the import decl exceeds the line length. The comment included with the existing tests says that import decls shouldn't wrap (and I take this to also mean that discretionary breaks should be removed if present). Thus, I have made additional changes to ensure that import statements never wrap in any situation, please correct me if this is not the desired behaviour.

To achieve this behaviour (see snippet below), I had to modify the disableBreaking printer control to be able to configure whether discretionary breaks should be allowed when breaks are suppressed (currently they are, but for the case of import decls this needs to be overridden to ensure that imports never wrap).

// Input
@_spi(
  STP
)
@testable
import testModule

// Output
@_spi(STP) @testable import testModule

I can see people wanting to be able to write code that wraps between attributes (as is sometimes done with @resultBuilder on struct decls, or property wrappers). Should this be made configurable, or is it bad style?

I also fixed the typo of isBreakingSupressed (to isBreakingSuppressed) because it was only used in 3 places or so, which imo makes a merge conflict very unlikely. I'm happy to revert that if requested.

Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! I agree that the desired behavior (for now, at least) is to forbid imports from wrapping under any circumstances, for consistency.

I'll kick off the CI over in swiftlang/swift-syntax#1086 (since we don't have the checks enabled on this repo yet).

@stackotter
Copy link
Contributor Author

Great, thank you! Ignore my question about companion PR in another thread, you already explained it here 😅

allevato added a commit to allevato/swift-syntax that referenced this pull request Apr 10, 2023
This is a companion to swiftlang/swift-format#501,
which tweaks the breaking behavior for `import` decls (and also fixes
this existing mis-styling).
@allevato allevato merged commit 4b7b315 into swiftlang:main Apr 10, 2023
allevato added a commit to allevato/swift-format that referenced this pull request Jun 29, 2023
Fix formatting of import with multiple attributes (fixes swiftlang#445) and ensure that imports never wrap
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.

None yet

2 participants