maxchuquimia / xcgrapher

Framework-level dependency graph generator for Xcode projects - with support for custom graphing
101 stars 8 forks source link

Lint Project #5

Closed rogerluan closed 3 years ago

rogerluan commented 3 years ago

Description

This PR is a proposal to lint the project, but not necessarily using the rules that I've applied myself.

I decided to lint the project because I noticed some code style weirdness that I have a personal preference to fix πŸ™ˆ and that are easy enough for a linter to resolve. So I decided to run 3 linters:

It's important to notice that I ran them in the order I've written above: first SwiftLint, the SwiftFormat, then manual. This is important because SwiftLint default rule set undoes some of the changes executed by SwiftFormat's default rule set (namely trailing commas in multiline dictionaries/sequences, in this particular project).

If you agree with this proposal, we can merge it as is and then add customization config files for each of the linters, and eventually perhaps automate the linting process somehow. Also, if this gets approved, then I can lint #7 as well 😊

maxchuquimia commented 3 years ago

Hahaha... okay, we can do this πŸ’ͺ

I find it better to be as explicit as possible about these rules because things change between versions and this has caused trouble with multiple devs in the past. I like something like this for SwlftLint....:

opt_in_rules:
    - closure_spacing
    - overridden_super_call
    - redundant_nil_coalescing
    - prohibited_super_call
    - explicit_init
    - unused_import
    - empty_parameters
    - attributes
    - closure_end_indentation
    - collection_alignment
    - trailing_comma
    - convenience_type
    - empty_collection_literal
    - empty_count
    - empty_enum_arguments
    - empty_string
    - extension_access_modifier
    - file_name_no_space
    - implicit_getter
    - implicit_return
    - joined_default_parameter
    - modifier_order
    - multiline_arguments
    - multiline_literal_brackets
    - multiple_closures_with_trailing_closure
    - operator_usage_whitespace
    - pattern_matching_keywords
    - prefer_self_type_over_type_of_self
    - unneeded_parentheses_in_closure_argument
    - multiline_arguments_brackets 
    - multiline_function_chains

disabled_rules:
    - file_length
    - line_length
    - type_body_length
    - identifier_name
    - type_name
    - for_where
    - todo
    - nesting
    - duplicate_enum_cases
    - force_unwrapping
    - force_cast
    - trailing_whitespace
    - function_body_length
    - cyclomatic_complexity
    - weak_delegate

custom_rules:

  customrule_case_let:
      regex: "case +\\.[A-Za-z0-9]*\\([^\n]*?(let )"
      message: "Move 'let' to directly after 'case'"
      capture_group: 1

  customrule_force_vertical_alignment_with_whitespace:
      regex: "[^ ]( {2,20})[:=]"
      message: "Unexpected whitespace - reconsider spacing!"
      capture_group: 1

  customrule_string_contentsof:
      regex: "String\\(contentsOf:"
      message: "This could make a synchronous API call - reconsider or use String(contentsOfFile:) for loading local files"

  customrule_init_nscoder:
      regex: "required init[^\n]*: NSCoder\\) \\{(\n +[fp]| \\S+\"| precon)"
      message: "Simplify to 'required init?(coder: NSCoder) { fatalError() }'"
      capture_group: 1

  customrule_blank_line_opening_class:
    regex: "(class|extension) [A-Z][^{\n]*\\{[^\n}]*\n^[^\n]"
    message: "Insert blank line after class/extension opening '{'"

  # This is a bit of a cheeky one as it's only checking top-level classes.. but should still work as a teaching mechanism
  customrule_blank_line_closing_class:
    regex: " *(class|extension) [A-Z][^\n]*\n(\n    [^\n]*|\n)+?\\}\n(\\})" 
    message: "Insert blank line before class/extension closing '}'"
    capture_group: 3

included: # paths to include during linting. `--path` is ignored if present.

excluded: # paths to ignore during linting. Takes precedence over `included`.
    - vendor
    - "*.h"
    - "*.m"
    - .build
    - Tests/SampleProjects

large_tuple:
    warning: 5

trailing_comma:
    mandatory_comma: true

multiline_arguments:
    first_argument_location: next_line

(I'm a massive fan of blank lines at the top and bottom of extensions... they are pretty much taking over classes these days)

And for SwiftFormat:

--swiftversion 5

# format
--ifdef no-indent
--allman false
--elseposition same-line
--guardelse same-line
--wrapcollections before-first
--wraparguments before-first
--wrapparameters before-first

# file options
--exclude Tests/SampleProjects, .build

# rules
--enable AnyObjectProtocol,blankLinesAroundMark,blankLinesBetweenScopes,consecutiveBlankLines,consecutiveSpaces,duplicateImports,emptyBraces,hoistPatternLet,leadingDelimiters,linebreakAtEndOfFile,linebreaks,modifierOrder,preferKeyPath,redundantBackticks,redundantBreak,redundantExtensionACL,redundantFileprivate,redundantGet,redundantInit,redundantLet,redundantLetError,redundantNilInit,redundantObjc,redundantParens,redundantPattern,redundantSelf,redundantVoidReturnType,semicolons,spaceAroundBraces,spaceAroundBrackets,spaceAroundComments,spaceAroundGenerics,spaceAroundOperators,spaceAroundParens,spaceInsideBraces,spaceInsideBrackets,spaceInsideComments,spaceInsideGenerics,spaceInsideParens,strongOutlets,strongifiedSelf,todos,trailingSpace,typeSugar,void,wrap,wrapAttributes,isEmpty

--disable wrapMultilineStatementBraces,redundantReturn,sortedImports,numberFormatting,unusedArguments,andOperator,blankLinesAtEndOfScope,blankLinesAtStartOfScope,initCoderUnavailable

If you prefer, I can go through and make these changes over the weekend - regarding automation, we may as well do it properly from the start (we can have a pre-build phase in the scheme that prints warnings to a file before building, that way it's clear before committing that there are outstanding style guide issues).

rogerluan commented 3 years ago

Oh boy. πŸ˜‚ Alright, let's see:

opt in rules: Didn't review, anything there LGTM opt out rules: I'd like to enable for_where, trailing_whitespace, and weak_delegate πŸ™ˆ customrule_case_let and customrule_force_vertical_alignment_with_whitespace: I don't understand what these do (and their messages are not clear to me) 😬 maybe we could rephrase the message to make it more clear? customrule_init_nscoder really like the idea of this one! Although I'd probably ~suggest~ require the @available(*, unavailable) declaration attribute on those initializers, if we're gonna do the right thing πŸ˜‰

customrule_blank_line_opening_class and customrule_blank_line_closing_class: omg πŸ™ˆ these were one of the biggest reasons that triggered my OCD to initiate a linting process in the project 🀣 but if you feel strong about them, you're the owner of the project after all, I'll leave that call up to you πŸ˜…

excluded: Tests/SampleProjects I'd remove this, I'd actually lint the sample projects and SPMs as well πŸ‘

And I generally like to enable sortedImports, blankLinesAtEndOfScope, blankLinesAtStartOfScope, initCoderUnavailable as well 😊

What do you think? We'll have to compromise on some of them perhaps πŸ˜‚


Automation: I learned to dislike build-time linters because they cost a lot in terms of performance (several seconds per build, potentially thousands of builds per month/year, adds up a lot), and its purpose is mostly to educate developers. I'm gonna assume that given the context of an open source project there's no need to educate contributors, which's a different scenario from working in the same project everyday in a company, in your 9-5, for example πŸ˜ƒ I particularly like to use SwiftFormat alongside https://github.com/hallettj/git-format-staged (check it out, it's amazing!), and for everything else I think we could simply abort the commit (on precommit hook) if there're any linter warnings. If you wanna go a step beyond, you can enable static code review bots/services to post on PRs, pointing out swiftlint/swiftformat warnings 😊 But to be honest I think automation can be done in a 2nd take, and initially it could be just a "hey, here are the config files, please run the linters" good-faith-kinda deal πŸ˜„

maxchuquimia commented 3 years ago

πŸ˜‚ yuppp

opt out rules: Okay, definitely happy to enable for_where and trailing_whitespace. We can enable weak_delegate too until it becomes a problem πŸ˜‚ (I guess most likely won't for this project anyway)

customrule_case_let - this one warns on a case like:

case .something(let varname):

and tells you to change to:

case let .something(varname):

(the reason being the latter formatting allows for multiple variables with a single let, e.g. case let .something(varA, varB))

customrule_force_vertical_alignment_with_whitespace - this (from memory) prevents the following scenario:

case .short         : return x
case .something     : return a
case .somethingElse : return b

The problem here is that if another case needs to be handled that is longer, e.g. .somethingWithALongName then the engineer will go and edit all the other cases to push the : return x rightwards to align with the new rightmost : - not great because it results in history being re-written for lines that are unrelated to the commit.

I'm more than happy for you to change the messages to something that makes more sense and add comments!

customrule_init_nscoder - good idea, go for it!

customrule_blank_line_*_class 😬😬😬😬 Sorry.... I'm not gonna sleep at night if those spaces aren't there πŸ˜‚

excluded: Tests/SampleProjects Fair enough, let's lint them (but ensure the Pods directory is excluded then)

sortedImports, blankLinesAtEndOfScope, blankLinesAtStartOfScope, initCoderUnavailable - Yup okay, sounds good to me

Thank for the suggestions!


Regarding automation, what you're saying makes perfect sense - perhaps we just add a lint.sh with the commands in the correct order?

rogerluan commented 3 years ago

customrule_case_let makes sense - good call! I didn't know about that syntax prior to working on this project πŸ˜ƒ your rationale to use it is πŸ’― customrule_force_vertical_alignment_with_whitespace the case which this linter rule protects against is absolutely disgusting, why would anyone ever do that πŸ˜† That'd be Ruby-like code style IIRC haha. So, ofc this rule LGTM! customrule_blank_line_*_class: sounds like one of us will have to lose sleep then 🀣 but don't worry, I give in. I find sleeping overrated anyway 😜

To all your other comments, incl. lint.sh, SGTM! πŸŽ‰ πŸš€ Let's do this!

maxchuquimia commented 3 years ago

Great! Happy to merge once we get the lint.sh and the configurations committed - I guess you have them locally on disk and just need to commit? Or would you like me to do it?

rogerluan commented 3 years ago

I'll disable weak_delegate actually, I remember what false-positives they used to cause now lol πŸ™…

rogerluan commented 3 years ago

@maxchuquimia I've also enabled andOperator rule on SwiftFormat, I hope that's fine. All the other rules were implemented according to what we had discussed πŸ‘ except there's still this issue:

image

This is being caused by SwiftFormat. Could you figure out a way for us to disable that behavior, while not disabling other important linters? Idk exactly which rule is causing that 😬 there's a bunch of whitespace rules in SwiftFormat. πŸ™

maxchuquimia commented 3 years ago

Great idea to add linting into the makefile! I think we need to run swiftlint . after the --autocorrect call so that it prints out the custom rule violations (as they won't be auto corrected).

Regarding andOperator - not a fan to be honest as if we extract some rules from a statement into a variable we can't say let x = a, b, we would say let x = a && b. Usually we just stick to , if the statement requires unwrapping something... Google doesn't explicitly mention this in their style guide (which was my starting point for creating a ruleset) but if you take a look they also don't seem to favour , unless unwrapping (https://google.github.io/swift/ - see the code in Control Flow Statements)

I did some digging and it seems blankLinesAtStartOfScope and blankLinesAtEndOfScope (from SwiftFormat) are causing the issues in your screenshot! Can we move them to the disabled section of the config file?

rogerluan commented 3 years ago

@maxchuquimia I've addressed those changes, except the andOperator one: I lean towards using commas instead of && where applicable (let x = a && b doesn't apply to those cases) simply because it's more Swifty (similar to Ruby's and operator instead of using &&, although both are valid syntax, and Swift's isMultipleOf(:) instead of the mod (%) operator).

That being said, I don't mind removing that rule. Let me know if you still want me to 😊

maxchuquimia commented 3 years ago

Okay let's leave it there for now and see how we go, ready to merge when you see fit!