Skip to content

[AMSDK-9723] [Tools] Setup SwiftLint #50

Merged
kevinlind merged 21 commits intoadobe:devfrom
kevinlind:amsdk-9723
Jul 18, 2020
Merged

[AMSDK-9723] [Tools] Setup SwiftLint #50
kevinlind merged 21 commits intoadobe:devfrom
kevinlind:amsdk-9723

Conversation

@kevinlind
Copy link
Copy Markdown
Contributor

Description

Setup SwiftLint for project. NOTE: Does not include CircleCI changes which will be included in another PR.

Part of this checkin are SwiftLint autocorrections and manual corrections to fix SwiftLint errors. The following is a list of the relevant changes.

  • Add Makefile target in ./Makefile to install SwiftLint from Homebrew and setup pre-commit Git hook
  • Add tools/git-hooks/pre-commit to run SwiftLint autocorrect on all files in Git stage area before being committed.
  • Add tools/git-hooks/setup.sh to copy pre-commit file to .git/hooks/pre-commit. Script is run from Makefile's setup-lint-tool target
  • Add tools/format/.swiftlint.yml SwiftLint configuration file. This file contains several changes to the default SwiftLint configurations, including enabling several opt-in rules, disabling the "nesting" rule, and setting "force_cast" and "force_try" rules to warnings instead of errors.
  • Update Xcode project for AEPExperiencePlatform to include Build Phase Run Script for each target to run lint on the target's source files. Each target has their own run script which lints the relevant files for that target (i.e. source, unit tests, functional tests, demo apps).
  • Ran SwiftLint autocorrect on all files under code using the configuration under tools/format/.swiftlint.yml. Also, manually corrected some errors reported by SwiftLint which were not auto-correctable. Note, I did not fix all the warnings produced by SwiftLint, just the errors in order for the project to build.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

kevinlind added 16 commits July 16, 2020 11:28
New Makefile target 'setup-lint-tool' will install SwiftLint and a Git pre-commit hook.
SwiftLint is installed via Homebrew. The Git hook is stored under 'hooks/pre-commit' and is copied to '.git/hooks/pre-commit'.
Add 'lenient' flag to unit test and functional test lint runs to treat all errors as warnings.
…t-files flag to autocorrect only the files in Git's stage area.
Makefile Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we rename this to setup tools to fit more tools in the future and maybe include it in make setup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same, however the tools setup can take several seconds as it runs Homebrew. I would prefer to use this make setup target in CI and as a documented way to setup the project once first cloned. If, when changing branches and wanting to correct the installed Pods, then make pod-install should be used. Thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess it's fine to not include it in make setup, you only need to run it once. I will keep my comment for renaming to make setup-tools

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might need to reduce these a bit. hope we don't have many functions with 300+ lines

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting read, seems like protocol ClassName: AnyObject is preferred though and class is deprecated without a warning https://forums.swift.org/t/class-only-protocols-class-vs-anyobject/11507

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this, I feel less stressed about formatting the lines manually 😄

@kevinlind kevinlind merged commit 5aed87d into adobe:dev Jul 18, 2020
@kevinlind kevinlind deleted the amsdk-9723 branch July 18, 2020 00:52
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