[Cleanup/Pipeline] Add Swiftlint

misteu / VocabularyTraining

Simple vocabulary Trainer written in Swift

Apache License 2.0

22 stars 15 forks source link

[Cleanup/Pipeline] Add Swiftlint #25

Closed misteu closed 1 year ago

misteu commented 1 year ago

GOAL

Swiftlint should be added as a new run script phase as described here: https://github.com/realm/SwiftLint and issues found by it should be fixed as well.

Optionally: If you have experience with Github Actions, feel free to add SwiftLint to the PR pipeline as well :)

WHY

To cleanup the project a little bit.

ACCEPTANCE CRITERIA

SwiftLint should be added to the project's build phases
The project should have no warnings anymore.

bladebunny commented 1 year ago

I can take this. Can you assign to me? Will submit a PR today. I'll update the Contribute section as well to include SwiftLint install guidance. For SwiftLint, will use the default rules for now.

misteu commented 1 year ago

Hey @bladebunny ! Thanks for your interest in this issue!

I just fixed the warnings and updated the project to Swift 5. I'll push/merge that soon. You can continue with adding SwiftLint + fixing these.

Also great idea to add it to the contribute section!

misteu commented 1 year ago

@bladebunny I assigned it to you but please wait a few minutes until my branch is merged so that you have the latest state before adding SwiftLint :)

misteu commented 1 year ago

It was automatically closed by my PR, I updated the issue a little bit and reopened it.

Feel free to start, whenever you have time :) Thank you!

bladebunny commented 1 year ago

@misteu Great. I'll pull your changes and add SwiftLift.

bladebunny commented 1 year ago

@misteu Just an FYI, have it added but its triggering a boat load of warnings now. I haven't add a lint rules file yet so it's using the defaults. I will fix as many warnings as I can. Most of the warnings are:

trailing whitespace violation: method(param:val) vs method(param: val)
line space warning (lines too long)

I will leave the two space indentation and add a rule for it.

This will be a big PR because of all the current lint violations.

misteu commented 1 year ago

Thank you!

Yeah I thought there will be a lot of warnings 🥲

Then please just add SwiftLint and keep them unfixed if there are too many. I agree, if it's becoming a really huge PR with just minor changes, I can fix them after your addition of Swiftlint.

misteu commented 1 year ago

I mean if you already fixed half of them or so, include as many as you fixed in this PR ;) I just don't want to leave you alone with this cleaning up.

bladebunny commented 1 year ago

@misteu Thx. I fixed a bunch but there are a handful left of:

Gnarly string formatters
Forced unwraps
TODOs ..that I'm not sure how to fix or probably require some more thought. I can probably fix some of those as well with some guidance but should probably be a separate issue -- IMHO.

bladebunny commented 1 year ago

PR added: https://github.com/misteu/VocabularyTraining/pull/29 Wound up not being too many changes after tuning the rules -- they can be tightened (such as line count/length) as you get the other warnings resolved.

bladebunny commented 1 year ago

FYI I don't think the remaining warnings will be difficult at all - it was more that I'm not that familiar with the app so was reluctant to make changes I could not easily verify. And there are different options for some of them so room for different approaches.

misteu commented 1 year ago

I will close it now as SwiftLint is added now to the project itself and is working with the github actions as well.

Thank you!