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

enabled swiftlint & fixed issues #657

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

enabled swiftlint & fixed issues #657

wants to merge 2 commits into from

Conversation

bryce-b
Copy link
Member

@bryce-b bryce-b commented Dec 14, 2024

I've added the swiftlint plugin to the 5.9 package.swift. I've also added .swiftlint and disabled several rules that are less simple to resolve, that we may revisit in the future.

@bryce-b
Copy link
Member Author

bryce-b commented Dec 14, 2024

It looks like I'll need to review swiftlint & the github actions. I'll investigate this in the new year.

@ArielDemarco
Copy link
Contributor

My 2 cents on the swiftlint usage: If the primary consumer of OpenTelemetry-Swift also uses SwiftLint, you’re likely to encounter many integration issues if they're using SPM.
Beyond the fact that when you download a dependency with SPM, it pulls in all its dependencies (meaning SwiftLint would also be downloaded in this case), one of the main issues with SwiftLint is that, aside from being heavy as a dependency, it’s also not stable and introduces many breaking changes from version to version (you can check on their releases they even include a Breaking changes section)

As a result, I believe this will cause significant integration conflicts for OpenTelemetry with many apps due to SwiftLint's instability and how unfriendly SPM is when handling heavy dependencies (Sadly, I speak from experience).

What I'd do is find other ways to enforce Swiftlint rules without exposing Swiftlint as a plugin in the Package.swift, either with a pre-commit hook (which could also --fix/--autocorrect issues) or other CI tooling.

@bryce-b
Copy link
Member Author

bryce-b commented Jan 6, 2025

@ArielDemarco Thanks for the feedback. I'll investigate a different way to utilize swiftlint

@bryce-b bryce-b closed this Jan 6, 2025
@bryce-b bryce-b reopened this Jan 9, 2025
@bryce-b bryce-b closed this Jan 9, 2025
@bryce-b bryce-b reopened this Jan 9, 2025
@bryce-b
Copy link
Member Author

bryce-b commented Jan 9, 2025

@ArielDemarco I really like being able to use swiftlint in xcode, and I think I've come up with a solution!
Using an env-var when loading xcode to enable it, it will not show up in the dependencies for consumers of our package, and it will not conflict with consumers that also use swiftlint. I've added a note on how to enable it locally in CONTRIBUTING.md. I'm also intending to add an optional pre-commit hook as well as a PR CI job.

@ArielDemarco
Copy link
Contributor

Looks great! I'll give it a try on a sample project but seems it'll work just fine!

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