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.

Sign up for GitHub

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

Use Apple's GitHub Workflows linting script #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a6d1b49
Use Apple's GitHub Workflows linting script
MahdiBM Dec 14, 2024
cf5390b
try again the old config
MahdiBM Dec 14, 2024
b6730bc
revert to the new code
MahdiBM Dec 14, 2024
c8cf304
format
MahdiBM Dec 14, 2024
f5cdfb3
try smth
MahdiBM Dec 14, 2024
8a12218
Revert "try smth"
MahdiBM Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .github/workflows/run-unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,10 @@ jobs:
if: ${{ inputs.with_linting }}
runs-on: ubuntu-latest
container: swift:6.0-noble
timeout-minutes: 30
timeout-minutes: 5
steps:
- name: Configure git
run: git config --global --add safe.directory "${GITHUB_WORKSPACE}"
- name: Check out code
uses: actions/checkout@v4
- name: Read config file
Expand All @@ -256,7 +258,9 @@ jobs:
curl -O https://raw.githubusercontent.com/vapor/contributing/refs/heads/main/.swift-format
fi
- name: Lint
run: swift-format lint --recursive --strict --parallel .
run: |
apt -q update && apt -yq install curl
curl -s https://raw.githubusercontent.com/swiftlang/github-workflows/refs/heads/main/.github/workflows/scripts/check-swift-format.sh | bash

musl:
if: ${{ !(github.event.pull_request.draft || false) && inputs.with_musl }}
Expand Down
1 change: 1 addition & 0 deletions sample-testable-package/Tests/SampleTestablePackageTests/SampleTestablePackageTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 •
edited
Loading

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.)

@testable import SampleTestablePackage

final class SampleTestablePackageTests: XCTestCase {
Expand Down
Loading