Add linting checks to CI process

wkyoshida commented 9 months ago

Terms

[X] I have searched open and closed issues
[X] I agree to follow Scribe-iOS' Code of Conduct

Issue

This issue is for setting up linting checks on PRs as part of our CI process. Also related to CI:

Issue derived from PR discussion

wkyoshida commented 9 months ago

Ideas could be (though open to discussion):

to use SwiftLint
for a GitHub action to display warnings on violations
for a GitHub action to notify the contributor to manually fix violations and/or automatically fix them by running linting themselves when applicable

andrewtavis commented 9 months ago

Thanks, @wkyoshida! I'll leave this for a moment, but can also get to it myself if it's not picked up :)

shashankiitbhu commented 7 months ago

Hi @wkyoshida @andrewtavis can I work on this issue? Really Interested in this overall project and would love to start contributing to it by this

andrewtavis commented 7 months ago

Makes sense to me! Feel free to do some research on this, @shashankiitbhu, and we can then discuss this a bit in the dev sync on Saturday 😊

andrewtavis commented 6 months ago

Hey @shashankiitbhu 👋 Doing a quick check in on this issue :) Anything we can do to help?

henrytwagner commented 6 months ago

Hello, I would definitely be interested on working on this if help is needed!

andrewtavis commented 6 months ago

Currently on a call with @henrytwagner to discuss a potential issue that could be worked on for a class on debugging and quality assurance. This issue came to mind as something that would be fitting to work on 😊 I'll reassign this issue under the circumstances :)

@henrytwagner, as discussed, what it is that we're looking for here is:

Find Swift repositories that have tests running on their PRs
Find ones that have linting and/or formatting checks
Check the .yaml configuration file for the tests that will be in the .github/workflows directory of the given project (the name of the test on the PR will be the same name as the file)
Investigate how those tests are being ran and first report back with some ideas of projects that we could model our tests on
- Note that the formatting extension that I'm using for the codebase so far is SwiftFormat
We can then go through with implantation from there
If the tests first pass for the repository, then we can also open a PR with broken .swift files to show that the process is functioning

@shashankiitbhu, we'd be happy to work with you in the future. Please let us know if there's another issue that you'd like to work on!

andrewtavis commented 5 months ago

Hey there @henrytwagner 👋 Just checking in here :)

Some ideas for some workflows that this could be based off of are:

Let me know if there's anything we can do to help on our end!

henrytwagner commented 5 months ago

Hello @andrewtavis,

I have been working on this most of the day today and feel like I finally have a good lay of the land. I had quite a few complications getting everything setup so that I can test the effectiveness of the workflows I am trying to implement but I have now smoothed those issues over and I am currently experimenting with a few different linting and formatting tools just to get a good understanding of them.

I will definitely check out those workflows out over the next couple days and I will let you know if I have any questions or need any advice!!

andrewtavis commented 5 months ago

All sounds great, @henrytwagner :) Looking forward to seeing what you come up with!

henrytwagner commented 5 months ago

Hello @andrewtavis,

I have a good understanding of how SwiftLint works and plan to use that as the main linting method but when I ran SwiftLint with the default settings it propagated with the errors and warnings I have listed below. I can set it so that it rejects or accepts when there are just warnings but what I really need help on is figuring out which settings to tweak. As of right now when you run SwiftLint with no config file you get these errors/warnings:

WARNING

14x function_body_length (50 lines or less) 112x line_length (120 characters or less) 154x trailing_comma (Collection literals should not have trailing commas) 13x cyclomatic_complexity (complexity 10 or less ) 8x identifier_name (range [3,40] characters) 8x todo (TODOs should be resolved) 4x file_length (File should contain 400 lines or less) 2x orphaned_doc_comment (Orphaned Doc Comment Violation) 28x opening_brace (Opening Brace Spacing Violation) 2x blanket_disable_command (Blanket Disable Command Violation) 1x type_body_length (Type Body Length Violation: Type body should span 250 lines or less excluding comments and whitespace) 6x for_where (Prefer For-Where Violation) 6x trailing_whitespace (Trailing Whitespace Violation) 5x vertical_whitespace (Vertical Whitespace Violation) 1x void_function_in_ternary (Void Function in Ternary Violation) 1x empty_enum_arguments (Arguments can be omitted when matching enums with associated values if they are not used) 1x closure_parameter_position (Closure parameters should be on the same line as opening brace)

366x TOTAL ERRORS

=========================

ERROR

6x cyclomatic_complexity (complexity 10 or less ) 30x identifier_name (range [3,40] characters) 8x function_body_length (50 lines or less) 9x force_cast (force casts should be avoided) 3x force_try (Force tries should be avoided) 1x type_body_length (Type body should span 350 lines or less excluding comments and whitespace) 5x line_length (Line should be 200 characters or less) 1x file_length (File should contain 400 lines or less)

63x TOTAL ERRORS

I'd be happy to go through and fix the current issue but for the most part I think it's just a matter of tweaking settings to fit this projects current formatting. Please let me know what you would like to have configured and what would be better to fix and then enforce moving forward.

Thank you!!

andrewtavis commented 5 months ago

This is great, @henrytwagner! Maybe what we can do now is set up the PR check and open one? With that we'll be able to see the errors together, and myself and some of the other iOS contributors can help fix them. Not a surprise that we have so many errors, and let's look to try to fix them before ignoring 😊 Definitely not failing on warn would be good though as there doubtless will be some things that will be too tough to change (at least for the first PR).

Maybe send along your workflow YAML file code when you have it, and then we can check it and give the go ahead for the PR? :)

Thanks so much!

henrytwagner commented 5 months ago

Hello @andrewtavis!

I have attached a linting .yaml file (pr_swift_lint.yaml) and its configuration file (.swiftlint.yaml). While I was working on this I placed the configuration file in /Scribe-iOS/.github/workflows/ right next to the linting file so the code could require a few easy adjustments if you think there's a better spot for the config.

You can see that the actual pr_swift_lint.yaml file Installs Homebrew, jq, and SwiftLint (I don't know if this actually needed as the latter two have already been installed each time I've tested). It then runs SwiftLint and tracks the number of warnings and errors as well as what and where they are. This is then all displayed with the quantity of each being shown at the end. Finally there is a message about the outcome of the linting check which should probably be updates to explain the result and what needs to be done better. This outcome is not obvious without checking the actual process but I'm sure there is a way to make the outcome more prominent (for example adding it as a bot comment like the maintainer checklist) it's just not something I have tried yet.

Once we tune the output messages and finalize the structure of that file, what really becomes important is the config file .swiftlint.yaml. Right now the only change I have made from the default configuration is allowing one character variable but you can see a bunch of different settings that can be tweaked are commented out. These are specifically the warnings/errors that are flagged with the current code base but there are many more that could be manipulated as needed. Additionally, as of right now code with errors will not pass but code with just warnings will, although this can easily be changed.

Please let me know if you have any thoughts. I'll also go ahead and attach what the current output looks like with the attached linting and config files.

Take off .txt extension for .yaml files swiftlint.yaml.txt pr_swift_lint.yaml.txt outputWithCurrentLintingandConfig.txt

henrytwagner commented 5 months ago

@andrewtavis, I just realized that swiftlint.yaml did not save as intended and should actually be titled ".swiftlint.yaml" with the dot at the beginning. I apologize for the potential confusion.

andrewtavis commented 5 months ago

No stress, @henrytwagner!

andrewtavis commented 5 months ago

Closed via #417 with minor edits to adopt a pre-built GitHub action coming after :) Has been tested in #418 and #419 and it's working 😊 From here, the next step would be to start removing some of the disabled rules in .swiftlint.yml and fixing the errors they pick up. Would be happy to make some issues for that :)

Thanks @henrytwagner! Let me know if you'd like to help making and working on those issues :)

scribe-org / Scribe-iOS

Add linting checks to CI process #390

Terms

Issue