Skip to content

Add SwiftLint to build script#80

Closed
AJ9 wants to merge 2 commits intoSwiftGen:masterfrom
AJ9:master
Closed

Add SwiftLint to build script#80
AJ9 wants to merge 2 commits intoSwiftGen:masterfrom
AJ9:master

Conversation

@AJ9
Copy link
Copy Markdown
Contributor

@AJ9 AJ9 commented Jan 3, 2016

As discussed in #79 and here, adding SwiftLint as a build script will reduce regression SwiftLint errors. This build script does check the entire SwiftGen project and I've added the following exclusions (to the .swiftlint.yml file) with the following reasons:

disabled_rules: 
  - force_try
  - force_cast 
  - variable_name 
  • force_try - Used in some unit tests to create result objects
  • force_cast - Used here and here
  • variable_name - Used sensibly here

@AJ9
Copy link
Copy Markdown
Contributor Author

AJ9 commented Jan 3, 2016

This does result in 242 issues, maybe we can find a way of just checking the expected folder?

@AliSoftware
Copy link
Copy Markdown
Collaborator

great work thx!
I think we might get rid of the try! calls by replacing them with guard try? … else { fatalError("some explicit message") } instead, which would both satisfy SwiftLint and make better error messages, as I hate killing ponies 😉

@AliSoftware
Copy link
Copy Markdown
Collaborator

AliSoftware commented Jan 3, 2016

This does result in 242 issues, maybe we can find a way of just checking the expected folder?

You can use swiftlint lint --path …/expected for that.

The main concern is indeed to ensure the code generated by SwiftGen match SwiftLint styling rules, so the expected folder is indeed the most important one to check. But it would be cool to make the codebase of SwiftGen itself cleaner by addressing those 242 warnings too.

I think we could use swiftlint lint --strict --path …/path/to/expected in the .travis.yml file, so that Continuous Integration only check the expected folder, and fail even on warning (--strict) to prevent new templates to introduce bad style.

Then we could use the simple swiftlint command (without --strict nor --path) in the Script Build Phase to check the whole codebase (and not just expected), and only warn (not fail); then (later?) address those lint warnings in the SwiftGen codebase itself.

@AliSoftware
Copy link
Copy Markdown
Collaborator

I just realized a problem: swiftlint won't be able to lint the files in UnitTests/expected because those files have the .swift.out extension, not just .swift, so they are discarded by SwiftLint and not analyzed. SwiftLint only analyze files having the .swift extension.

So I'm not sure there is a proper way to validate that the templates or their output does not violate SwiftLint rules. Maybe there is hope with the swiftlint lint --use-stdin somehow, but not with swiftlint directly analyzing the files.

@AliSoftware AliSoftware force-pushed the master branch 11 times, most recently from e8e8587 to 76aff13 Compare March 13, 2016 18:44
@AliSoftware AliSoftware force-pushed the master branch 3 times, most recently from d251ff7 to 3d4251e Compare June 2, 2016 00:30
@AliSoftware AliSoftware added this to the 1.1.0 milestone Jun 2, 2016
@AliSoftware
Copy link
Copy Markdown
Collaborator

Hi @AJ9

I finally took the time to merge this. In fact I reintegrated SwiftLint the way you started to do it (but in my own branch to start anew since your branch started to be a bit behind the latest merged PRs), and in addition I added linting for both the templates and the SwiftGen code itself, and fixed all the warnings & errors generated by the listing.

Thanks again for taking a stab at it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants