Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite cyclomatic_complexity with SwiftSyntax #5308

Merged
merged 6 commits into from
Oct 29, 2023

Conversation

marcelofabri
Copy link
Collaborator

No description provided.

@SwiftLintBot
Copy link

SwiftLintBot commented Oct 29, 2023

1 Warning
⚠️ This PR introduced a violation in Brave: /Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+Wallet.swift:192:10: warning: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 13 (cyclomatic_complexity)
18 Messages
📖 Linting Aerial with this PR took 1.18s vs 1.17s on main (0% slower)
📖 Linting Alamofire with this PR took 1.56s vs 1.55s on main (0% slower)
📖 Linting Brave with this PR took 8.86s vs 8.81s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 4.43s vs 4.44s on main (0% faster)
📖 Linting Firefox with this PR took 10.35s vs 10.38s on main (0% faster)
📖 Linting Kickstarter with this PR took 10.86s vs 10.85s on main (0% slower)
📖 Linting Moya with this PR took 0.62s vs 0.62s on main (0% slower)
📖 Linting NetNewsWire with this PR took 3.24s vs 3.23s on main (0% slower)
📖 Linting Nimble with this PR took 0.82s vs 0.82s on main (0% slower)
📖 Linting PocketCasts with this PR took 8.73s vs 8.66s on main (0% slower)
📖 Linting Quick with this PR took 0.4s vs 0.4s on main (0% slower)
📖 Linting Realm with this PR took 11.4s vs 11.44s on main (0% faster)
📖 Linting Sourcery with this PR took 2.82s vs 2.78s on main (1% slower)
📖 Linting Swift with this PR took 5.21s vs 5.21s on main (0% slower)
📖 Linting VLC with this PR took 1.55s vs 1.54s on main (0% slower)
📖 Linting Wire with this PR took 19.37s vs 19.34s on main (0% slower)
📖 Linting WordPress with this PR took 13.13s vs 13.04s on main (0% slower)
📖 This PR fixed a violation in Brave: /Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+Wallet.swift:192:10: warning: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 12 (cyclomatic_complexity)

Generated by 🚫 Danger

@marcelofabri
Copy link
Collaborator Author

I've looked into the only change oss-check is reporting and I think this PR is actually fixing a bug.

Looks like the old implementation could end up excluding fallthrough multiple times given how it'd call itself recursively without excluding fallthroughs that were already counted - the old impl also didn't guarantee fallthrough as a keyword :)


// Switch complexity is reduced by `fallthrough` cases
override func visitPost(_ node: CatchClauseSyntax) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a bit weird, and maybe not intentional but this snippet:

do {} catch {}

contains a case according to SourceKitten:

{
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.length" : 14,
  "key.offset" : 0,
  "key.substructure" : [
    {
      "key.bodylength" : 0,
      "key.bodyoffset" : 4,
      "key.kind" : "source.lang.swift.stmt.brace",
      "key.length" : 2,
      "key.offset" : 3
    },
    {
      "key.elements" : [
        {
          "key.kind" : "source.lang.swift.structure.elem.pattern",
          "key.length" : 1,
          "key.offset" : 12
        }
      ],
      "key.kind" : "source.lang.swift.stmt.case",
      "key.length" : 8,
      "key.offset" : 6
    }
  ]
}

since it's kind of a different branch I decided to keep counting it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that SourceKit thinks that's a case statement is weird, but it's true that there are two branches in do {} catch {}: one where only do is invoked and another where do and catch are invoked.

@marcelofabri marcelofabri marked this pull request as ready for review October 29, 2023 10:27
Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Good job! I had tried this one a while back and didn't crack it, but reading through this one looks correct to me!

@marcelofabri marcelofabri merged commit 715198a into main Oct 29, 2023
11 checks passed
@marcelofabri marcelofabri deleted the marcelo/cyclomatic_complexity-swift-syntax branch October 29, 2023 18:17
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.

3 participants