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

Rule Request: No Empty Function #5615

Closed
2 tasks done
Ueeek opened this issue Jun 4, 2024 · 2 comments · Fixed by #5617
Closed
2 tasks done

Rule Request: No Empty Function #5615

Ueeek opened this issue Jun 4, 2024 · 2 comments · Fixed by #5617

Comments

@Ueeek
Copy link
Contributor

Ueeek commented Jun 4, 2024

New Issue Checklist

New rule request

A Function body should not be empty. Otherwise, show a warning.

An empty function, init and deinit can harm readability and lead to confusion because readers need to guess whether it's intentional or not.
Developers should put a comment to explain why the function/ init / deinit body is empty.

SonarLint for Swift and ESLint have the same rule.
SwiftLint can be better by having this rule.

Triggering and Non-Triggering

Triggering:
func f1 () {}

func f2 () {
}

// Comment
func f3 () {}

init() {}

deinit {}
Non-Triggering
func f4 () {
    let s = "xyz"
}

func f5 () { /* Comment*/  }

func f6() {
    // Comment
}

init() { /* comment */ }

deinit { /* comment */ }

Just so you know, this behavior is the same as SonarLint and ESLint.

Should be configurable?

No parameter to configure.

Should be opt-in?

In my opinion, this rule should be "opt-in rule" since there is no consensus, though it's active by default in SonarLint and ESLint.
Got feed back, and change to default.

If this rule makes sense, I will work for this!
Thanks.

Future enhancement

We can implement rules for closures and initializers as well.
But, they should be implemented as separate rules.

@mildm8nnered
Copy link
Collaborator

Should empty inits and deinits be violations as well?

I actually have these as custom rules, but I think it would be nice if there was a built-in

I would personally lean towards this being on by default except that I think a lot of Xcode templates have empty functions in - OTOH maybe that's even more of a reason to make it a default.

  empty_functions:
   included: ".*.swift"
   name: "Functions must have a non-empty body"
   message: "Function bodies must have some content, even if it's just a comment"
   regex: 'func\s+[^{]+\{\s*\}'
   severity: warning
  empty_init:
    included: ".*.swift"
    name: "init methods must have a non-empty body"
    message: "Init function bodies must have some content, even if it's just a comment"
    regex: 'init[^{]+\{\s*\}'
    severity: warning

@Ueeek
Copy link
Contributor Author

Ueeek commented Jun 5, 2024

@mildm8nnered , Thank you for your feedback!
I also think it's a good idea to support init() and deinit.

OTOH maybe that's even more of a reason to make it a default.

I agree with this point.
Let me change it to the default rule from the opt-in rule.
If anyone has a different opinion, we can change it back to opt-in.

I updated PR and the above description based on your feed back!
Thank you!

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

Successfully merging a pull request may close this issue.

3 participants