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

Initial swiftlint file and fixes #781

Merged

Conversation

mildm8nnered
Copy link
Contributor

@mildm8nnered mildm8nnered commented Aug 4, 2024

So this branch has a .swiftlint.yml, and a lot of fixes - I went pretty aggressive here in enabling as much as possible, and also fixing the issues.

There are no really material code changes - many of them were, for example, the "imports weren't sorted", but if you'd prefer a less aggressive approach, let me know.

What there isn't here is any CI or Xcode integration.

@mildm8nnered mildm8nnered force-pushed the mildm8nnered-more-swiftlint-updates branch from a627097 to 56a4d1b Compare August 7, 2024 23:18
@mildm8nnered
Copy link
Contributor Author

Would love to get general feedback @ileitch if you think this it too aggressive an approach

Copy link
Contributor

@ileitch ileitch left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding this!

I think we can just drop the cyclomatic complexity and body/file length rules and merge as-is. I will go through later and tweak/fix the rules a little.

.swiftlint.yml Show resolved Hide resolved
.swiftlint.yml Outdated Show resolved Hide resolved
.swiftlint.yml Outdated Show resolved Hide resolved
Sources/Indexer/SwiftIndexer.swift Outdated Show resolved Hide resolved
@mildm8nnered
Copy link
Contributor Author

Thanks so much for adding this!

That's quite ok. Thanks for picking it up!

I think we can just drop the cyclomatic complexity and body/file length rules and merge as-is. I will go through later and tweak/fix the rules a little.

Shall I make the changes as per your comments, and then you can make any additional ones on top, just so we're not stepping on each others toes? I should be able to get to it tonight or tomorrow at latest ...

@ileitch
Copy link
Contributor

ileitch commented Aug 10, 2024

Shall I make the changes as per your comments, and then you can make any additional ones on top, just so we're not stepping on each others toes? I should be able to get to it tonight or tomorrow at latest ...

I think let's just remove the cyclomatic complexity and body/file length rules in this PR, then any other tweaks can be made in follow-up PRs.

@mildm8nnered mildm8nnered marked this pull request as ready for review August 10, 2024 11:49
@mildm8nnered
Copy link
Contributor Author

Shall I make the changes as per your comments, and then you can make any additional ones on top, just so we're not stepping on each others toes? I should be able to get to it tonight or tomorrow at latest ...

I think let's just remove the cyclomatic complexity and body/file length rules in this PR, then any other tweaks can be made in follow-up PRs.

Cool - should be good to go!

@ileitch ileitch merged commit a4b6cf1 into peripheryapp:master Aug 10, 2024
4 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