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

Use Apple's GitHub Workflows linting script #42

Merged
merged 6 commits into from
Dec 14, 2024
Merged

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Dec 14, 2024

The script is configured to respect a .swiftformatignore file which I'll need for Penny.
swift-format might soon have this ability in itself: swiftlang/swift-format#873

@MahdiBM MahdiBM requested a review from gwynne as a code owner December 14, 2024 03:42
@@ -1,4 +1,5 @@
import XCTest

Copy link
Member

Choose a reason for hiding this comment

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

Really? A spurious newline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually can't find out why this wasn't reported before. This is needed based on how swift-format behaves.

Did some tests locally, it would sometimes randomly emit a warning about this, and sometimes not.
I couldn't find out what makes it care about that line, and what makes it not. Tried different configurations as well
...

Copy link
Member

@gwynne gwynne Dec 14, 2024

Choose a reason for hiding this comment

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

I'm strongly against needing the newline, frankly... If swift-format is demanding it, to me that's a bug in swift-format. Imports should be grouped together unless there are a lot of them and they fall naturally into sectioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are at a point of "it is what it is" with swift-format. I'd suggest you give up as well. The configuration that Apple repos use also has the same behavior IIRC.

I think it requires the new line because the next import is a @testable one.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, obviously it does but to me it's visually jarring and completely pointless to have that gap. Why should @testable imports be separated?

(I know, I know, swift-format isn't gonna change... but there should really be more control.)

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

I must say there are a lot of ways in which I don't like the direction swift-format has taken. For all its issues, swiftformat always impressed me more with its flexibility.

@gwynne gwynne merged commit 1fd5aca into vapor:main Dec 14, 2024
11 checks passed
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.

2 participants