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

Enforce spaces in braces #640

Closed
ZevEisenberg opened this issue Apr 26, 2016 · 21 comments
Closed

Enforce spaces in braces #640

ZevEisenberg opened this issue Apr 26, 2016 · 21 comments
Labels
rule-request Requests for a new rules.

Comments

@ZevEisenberg
Copy link
Contributor

My team is using a particular curly brace style: extra newlines padding curly braces in classes, structs, enums, and extensions. We do not apply it to functions/conditionals/etc:

enum State {

    case Empty
    case Loading
    case Loaded

}

class ViewModel {

    var state: State

    func wazzup() {
        if state == .Empty {
            print("empty")
        }
        else {
            print("not empty")
        }
    }

}

My gut feeling is that this is not enforceable with custom rules as they currently exist. I also don't know how broadly something like this would be desired (you could also enforce not having the extra space, if you preferred the opposite style to my team). Is this something that would make sense as part of SwiftLint?

@jpsim
Copy link
Collaborator

jpsim commented Apr 26, 2016

This would definitely make sense as part of SwiftLint. I'd picture a configurable rule enforcing the number of padding newlines for types, with a default value of 1.

@jpsim jpsim added the rule-request Requests for a new rules. label Apr 26, 2016
@ZevEisenberg
Copy link
Contributor Author

I just realized an edge case: some people may want different numbers of newlines at the top and bottom. Since the Swift community has largely settled on the One True Brace Style (opening braces on same line), I know that some prefer to structure their code like this to make the first line of the function more readable:

func doSomething() {

    print("the first line is separated by a new line so it doesn't blend with the function declaration")
}

Not my preferred style, but worth considering in case it would impact the implementation of this feature. But, since you said it would be "padding newlines for types", maybe it's separate?

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 13, 2017

I know the discussion about this rule is quite old but I still agree that an opt-in rule (or two?), where spaces around braces could be configured would be a win for many projects. We are actually using a very simple rule which I described in #1518 using a custom_rule & that works fine for us. But I think such an option should be included into SwiftLint even if it is opt-in.

Although the rationale for our own style is simple ("if there's indentation, why bother and add a new line – it's clean and readable enough thanks to indentation already, prevent scrolling then!") I can understand that others have different styles that may even vary between type declarations and functions/conditionals. Note that for the type declarations part there has already been #1345 which was only closed due to inactivity some time ago.

So here's my suggestion (which I would also volunteer to implement):

  • Add a new rule named spaces_around_braces
  • Add config options for min/max outer and inner spaces counts
  • Possibility to optionally & separately apply to functions and conditionals
  • Allow the config options for spaces counts to specify "any"

Having the rule be this flexible, I think that we can even come up with a default configuration that is common ground within the Swift community so we can think about enabling the rule by default. The default configuration could for example look like this:

- spaces_around_braces:
  - opening_brace_padding:
    - outer:
      - min: 1
      - max: nil # any count
    - inner:
      - min: nil # any count
      - max: 0
  - closing_brace_padding:
    - inner:
      - min: nil # any count
      - max: 0
    - outer:
      - min: 1
      - max: nil # any count
  - apply_to_type_declarations: true
  - apply_to_function_declarations: true
  - apply_to_function_bodies: false

This is what big community frameworks like Alamofire or Moya are following and what I see most of the time in many Swift frameworks and Apple examples. To put it in examples, this would lead to:

// ❌ not acceptable
enum State {

    case empty
    case loading
    case loaded

}

// ✅ acceptable
enum State {
    case empty
    case loading
    case loaded
}

// ❌ not acceptable
class ViewModel {

    var state: State

    func wazzup() {
        if state == .empty {
            print("empty")
        }
        else {
            print("not empty")
        }
    }
    
}

// ✅ acceptable
class ViewModel {
    var state: State

    func wazzup() {
        if state == .empty {
            print("empty")
        }
        else {
            print("not empty")
        }
    }
}

// ❌ not acceptable
enum State {
    // some code
}
class ViewModel {
    // some code
}

// ✅ acceptable
enum State {
    // some code
}

class ViewModel {
    // some code
}

// ❌ not acceptable
class ViewModel {
    var state: State
    func wazzup() {
        // some code
    }
}

// ✅ acceptable
class ViewModel {
    var state: State

    func wazzup() {
        // some code
    }
}

// ❌ not acceptable
func wazzup() {

    if state == .empty {
        print("empty")
    }
    
    else {
        print("not empty")
    }
    
}

// ✅ acceptable
func wazzup() {
    if state == .empty {
        print("empty")
    }
    else {
        print("not empty")
    }
}

To alter the default configuration to have our own custom rule we would just need to add this to our .swiftlint.yml then:

- spaces_around_braces:
  - apply_to_function_bodies: true

To enforce what the OP @ZevEisenberg wanted to do, he would just need to configure it like this:

- spaces_around_braces:
  - opening_brace_padding:
    - inner:
      - min: 1
      - max: 1
  - closing_brace_padding:
    - inner:
      - min: 1
      - max: 1
  - apply_to_function_declarations: false

What do you think?

@marcelofabri
Copy link
Collaborator

What exactly apply_to_other_code would mean?

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 13, 2017

I couldn't find a good name for that, maybe you've got a suggestion? It is about all the other code where parentheses could be included like within a function like this (if it was turned on):

// ❌ not acceptable
func wazzup() {
    if state == .empty {
        print("empty")
    }
    state = .loaded
}

// ✅ acceptable
func wazzup() {
    if state == .empty {
        print("empty")
    }

    state = .loaded
}

@marcelofabri
Copy link
Collaborator

I think that for a first version of this rule, that option is not needed. That could be handled as an improvement later if people feel it's useful.

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 13, 2017

Well, the problem is: I need it. The reason why I want to implement this rule is to replace my custom rule. But without that option, I can't. This means I'll have no reason to implement it. ;)

@marcelofabri
Copy link
Collaborator

Cool, but I'd try to think about a better name for the key then 😅

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 13, 2017

I thought about apply_to_function_content but that missed the fact, that closures could hold such cases, too. Other things I thought about were apply_to_non_declarative_code or apply_to_non_declarations as well as apply_to_code_content. But I'm not really happy with any of them ... :-/ Maybe we have to rename them all to have something more clear?

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 13, 2017

By the way, I already renamed it to apply_to_inner_code (I just recognized) – I had changed that name several times, stopping with that since I couldn't come up with anything better.

@ZevEisenberg
Copy link
Contributor Author

How about apply_to_function_bodies? I don't think it's that confusing that it would also apply to closures. I think it's probably clear enough that "function bodies" refers to function-like constructs.

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 14, 2017

Well then, apply_to_function_bodies be it. Thanks for the suggestion.

I'll start implementing this sometime next week, hopefully.

What are your thoughts about turning it on by default? I'm sure it will add a lot of warnings in many peoples projects who don't adhere to the most common community Swift standards. This is a question of: Do we want to declare the default configuration a Swift community standard? Or which config do we want to declare the community standard? Cause if we add it by default, it probably will be a community standard – and as far as I've seen in many projects this is the default configuration which is pretty much common ground for most open source projects with many contributors. Again, see Alamofire and Moya as examples.

As a reminder, my default config suggestion is this:

- spaces_around_braces:
  - opening_brace_padding:
    - outer:
      - min: 1
      - max: nil # any count
    - inner:
      - min: nil # any count
      - max: 0
  - closing_brace_padding:
    - inner:
      - min: nil # any count
      - max: 0
    - outer:
      - min: 1
      - max: nil # any count
  - apply_to_type_declarations: true
  - apply_to_function_declarations: true
  - apply_to_function_bodies: false

If this default config was too strong, we could also weaken it by allowing one line of inner whitespace, then the rule would only make sure there's at least one outer whitespace line and no multiple inner whitespace lines. But I'm against this default behavior, personally, though I think it's better to have the rule turned on by default and if this is the price for that, I'm okay with that. The default config would look like this then:

- spaces_around_braces:
  - opening_brace_padding:
    - outer:
      - min: 1
      - max: nil # any count
    - inner:
      - min: nil # any count
      - max: 1
  - closing_brace_padding:
    - inner:
      - min: nil # any count
      - max: 1
    - outer:
      - min: 1
      - max: nil # any count
  - apply_to_type_declarations: true
  - apply_to_function_declarations: true
  - apply_to_function_bodies: false

What do you think?

@ZevEisenberg
Copy link
Contributor Author

I think it should be off by default, i.e. Everything should be nil. I don't think this is something most people think about, so it's mostly there for people who actively seek it out.

@marcelofabri
Copy link
Collaborator

I think the rule should be opt-in, but it could have sensitive defaults.

@Jeehut
Copy link
Collaborator

Jeehut commented Jun 14, 2017

I'm also for having sensitive defaults. The remaining question is what that "sensitive defaults" should be. My stricter first suggestion? My weaker second one? Or something entirely different?

@iliaskarim
Copy link

@Dschee where you at

@Jeehut
Copy link
Collaborator

Jeehut commented Jul 24, 2018

Oh, I just discovered this discussion again, I completely forgot about it. 😅

In the meantime, I created this issue and implemented two rules to fix it in this PR which is now waiting to be merged. So it shouldn't take very long anymore until it is merged and you can use it. It's not very configurable yet, but that shouldn't be hard to add over time.

@iliaskarim
Copy link

@Dschee that's awesome I can't wait to use vertical_whitespace_opening_braces and vertical_whitespace_closing_braces in my projects~

@jpsim
Copy link
Collaborator

jpsim commented Jan 22, 2019

Looks like we forgot to close this when we merged #2291.

@jpsim jpsim closed this as completed Jan 22, 2019
@gobetti
Copy link

gobetti commented Mar 18, 2019

hi folks, I'm not sure how to apply the rules from #2291 in order to solve what's originally reported in this issue: the rules seem to warn me if I do add lines after opening or before closing braces, but how do I get warned if I don't add lines before and after functions? thanks!

@Anavrin1990
Copy link

hi folks, I'm not sure how to apply the rules from #2291 in order to solve what's originally reported in this issue: the rules seem to warn me if I do add lines after opening or before closing braces, but how do I get warned if I don't add lines before and after functions? thanks!

I have the same question. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule-request Requests for a new rules.
Projects
None yet
Development

No branches or pull requests

7 participants