Skip to content

Conversation

crazytonyli
Copy link
Contributor

At the moment, SwiftLint issues are not super straightforward to discover. It's part of CI checks, but requires manually running a command locally. Also, after running the swiftlint command, it's pretty hard to identify exact locations of the issues by reading its console output.

This PR integrates SwiftLint plugin into the WordPressAPI module, so that we can see linting issue right from Xcode when building the package.

However, I don't want to add SwiftLint as part of the package dependencies, which would put extra constraints (also adding unnecessary swift packages) to the consumers. Since Swift Pakcage Manager treats all package dependencies as runtime dependencies, and there is no concept of "build dependencies", I have added some conditional checks in Package.swift to only add SwiftLint for "local development".

Since now we have different dependencies on local mac and CI agents, the Package.resolved content would be different too. I have checked in the Package.resolved file, with SwiftLint and other dependencies in it. But on CI, that file basically gets ignored. I think that should be fine for now. Let me know if you have any concerns.

@crazytonyli crazytonyli requested a review from jkmassel May 30, 2024 01:53
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

Running make lint-swift locally doesn't work for me after this change – it seems like 0.53.0 is the last version that's compatible with linux/arm64/v8 – am I doing it wrong?

@oguzkocer
Copy link
Contributor

Running make lint-swift locally doesn't work for me after this change – it seems like 0.53.0 is the last version that's compatible with linux/arm64/v8 – am I doing it wrong?

Fwiw, this wasn't working for me before this PR either. I had to manually setup swiftlint to get things working, but as @crazytonyli mentioned in the PR description, it was inconsistent with the information in the CI.

@crazytonyli
Copy link
Contributor Author

Yeah, neither 0.53 nor 0.55 work on my Mac. I thought we'll use we have the SwiftLint Swift Package locally, and use the docker image on CI.

Maybe we can get rid of the docker image and just use the SwiftLint package: swift package plugin swiftlint?

@crazytonyli
Copy link
Contributor Author

Maybe we can get rid of the docker image and just use the SwiftLint package: swift package plugin swiftlint?

@jkmassel I have implemented this ⬆️ in 687e43e. Let me know what you think 🙂

@crazytonyli crazytonyli requested a review from jkmassel June 6, 2024 23:39
@crazytonyli crazytonyli force-pushed the swift-package-add-swiftlint branch from 41e4b54 to 03b20d7 Compare June 10, 2024 22:57
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@crazytonyli crazytonyli enabled auto-merge (squash) June 13, 2024 01:03
@crazytonyli crazytonyli merged commit e4a6add into trunk Jun 13, 2024
@crazytonyli crazytonyli deleted the swift-package-add-swiftlint branch June 13, 2024 01:25
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.

3 participants