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: Vertical Whitespace Opening/Closing Braces #1518

Closed
Dschee opened this Issue May 16, 2017 · 14 comments

Comments

Projects
None yet
6 participants
@Dschee
Copy link
Collaborator

Dschee commented May 16, 2017

This is a rule request. I'm willing to implement this myself and send a PR, but before doing that I'd like to know if the rule is wanted or not. To explain the desired rule, I'd like to first post the custom rule we are already using in our projects:

  vertical_whitespace_opening_braces:
    included: ".*.swift"
    regex: '[{(\[][ \t]*\n[ \t]*\n'
    name: "Vertical Whitespace after Opening Braces"
    message: "Don't include vertical whitespace (empty line) after opening braces."
    severity: warning
  vertical_whitespace_closing_braces:
    included: ".*.swift"
    regex: '\n[ \t]*\n[ \t]*[)}\]]'
    name: "Vertical Whitespace before Closing Braces"
    message: "Don't include vertical whitespace (empty line) before closing braces."
    severity: warning

I felt like these rules might be useful for other projects as well, which is why I'm posting it. What do you think about it? For us it helps a lot to prevent common issues and enforce a shared style.

Rationale

We had this discussion a few times, the common ground we were all happy with is this: The main reason to use newlines is to have a visual clue about which parts of the code belong together. These rules together ensure you never have a newline where you should already use an indentation which is a good visual clue already, in this case due to an opening parentheses at the end of a line and a closing at the beginning.

A few examples:

let x = 5

// ❌ not acceptable
if x == 5 {

    print("x is 5")

}

// ❌not acceptable
if x == 5 {

    print("x is 5")
}

// ❌not acceptable
if x == 5 {
    print("x is 5")

}

// ✅ acceptable
if x == 5 {
    print("x is 5")
}

@Dschee Dschee changed the title Vertical Whitespace Opening/Closing Braces New Rule: Vertical Whitespace Opening/Closing Braces May 16, 2017

@Dschee Dschee changed the title New Rule: Vertical Whitespace Opening/Closing Braces Rule Request: Vertical Whitespace Opening/Closing Braces May 16, 2017

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented May 16, 2017

Related to #1345 and #640

@salutis

This comment has been minimized.

Copy link

salutis commented Jun 15, 2018

+1 and thanks @Dschee for posting the custom rules. 😍

@gobetti

This comment has been minimized.

Copy link

gobetti commented Jul 9, 2018

I felt like these rules might be useful for other projects as well, which is why I'm posting it

it would also be super useful for autocorrect! @Dschee do you still feel like adding this rule to SwiftLint? 🙏 otherwise let me know, I'm also interested 👍

@Dschee

This comment has been minimized.

Copy link
Collaborator

Dschee commented Jul 9, 2018

@gobetti I really wanted to but didn't have time til now. But recently I got some time and if my plans go as I hope then I should be able to add some rules with autocorrect to SwiftLint in about 2 weeks. But that shouldn't hold you off from implementing this right now if you have the time for it. I have plenty of rules I want to add (see here for the custom rules we use in every app project, and there's even more that we couldn't add via Regexes) so I would have more time for that. Your choice. ;)

@Dschee

This comment has been minimized.

Copy link
Collaborator

Dschee commented Jul 11, 2018

I just implemented this rule in #2291, including support for autocorrect. 🎉

@gobetti

This comment has been minimized.

Copy link

gobetti commented Jul 11, 2018

cool, I was wrapping up my implementation, but you beat me anyway... I'll try to help though by reviewing your implementation and testing it with my triggering and nonTriggering examples 👍

@Dschee

This comment has been minimized.

Copy link
Collaborator

Dschee commented Jul 12, 2018

@gobetti Sorry for saying you could implement it and then implement it myself – it just turned out I had some time left off ... thanks for helping out anyways!

@FrancoSabadini

This comment has been minimized.

Copy link

FrancoSabadini commented Jul 29, 2018

Thanks for these custom rules :)

I was thinking if there is an easy way to modify them to exclude class/struct definitions. I prefer to add empty lines between them as I find the code is cleaner to read that way, e.g.:

class Foo {

    func doSomething() {}

}
@Dschee

This comment has been minimized.

Copy link
Collaborator

Dschee commented Jul 30, 2018

@FrancoSabadini My current implementation doesn't have a configuration to exclude specific types of braces, but I think that this is a useful improvement once the basic feature is merged.

I suggest you open a new issue once #2291 is merged, but currently I don't have the time to implement this, I already invested quite some time improving SwiftLint rules and adding new ones and I hope the new rules will get merged without much additional work. I think to implement this one would need to change my implementation from a Regex-based solution to an AST-based one in order to be able to differentiate between the different kinds of braces – which would take some time ...

@Dschee

This comment has been minimized.

Copy link
Collaborator

Dschee commented Dec 3, 2018

@FrancoSabadini #2291 is merged now, feel free to improve it with your suggestions! :)

@FrancoSabadini

This comment has been minimized.

Copy link

FrancoSabadini commented Dec 3, 2018

Thanks @Dschee 👍, to be honest, I'm quite new with Swift and this library in general but will try to check it out and see if I can make it work when I have some time in the next couple of months :)

@Dschee

This comment has been minimized.

Copy link
Collaborator

Dschee commented Dec 3, 2018

@FrancoSabadini Oh ok, thank you for pointing that out. But in my opinion there's no better way learning a language than implementing a feature on an open source project where you get feedback about all important aspects as testing, readability and performance. So, if you have the time, feel free to still implement this as it's not an urgent feature and I'll try to give you feedback as much as I can.

To get you started, here are steps I think you should take if you agree to implement this:

  1. Read the CONTRIBUTING.md file
  2. Fork the project
  3. Checkout your fork in git & do all steps needed as documented in the CONTRIBUTING.md
  4. Run the tests (Cmd+U) and make sure they all pass and building works fine
  5. Add your expected behavior by altering the nonTriggeringExamples and violatingToValidExamples in the files VerticalWhitespaceClosingBracesRule and/or VerticalWhitespaceOpeningBracesRule
  6. Run the tests and make sure they are now failing (feel free to commit this change)
  7. Now change the implementation until all tests are passing again
  8. Add an entry to che CHANGELOG.md with a summary of your change
  9. Post a PR to get more feedback

Please note that we probably want to make this feature a configuration option on the rules. So you probably should also introduce a new configuration file under RuleConfigurations which looks similar to this configuration file. You probably want to call the option something like linesBefore/AfterType instead of maxEmptyLines and make it an [String: Int]. The keys could be method, property, subtype and other. The value would be the exact number of lines to ensure.

I hope this helps getting you started – good luck and feel free to ping me within a [WIP] PR in case you have questions.

@FrancoSabadini

This comment has been minimized.

Copy link

FrancoSabadini commented Dec 3, 2018

Thanks for all the information @Dschee :), it is certainly very useful to get me started so I really appreciate it. I will certainly give it a go when I have some time and I agree it'll be a good experience to learn more about Swift and this library in general.

@Kalvin126

This comment has been minimized.

Copy link

Kalvin126 commented Jan 4, 2019

Would be great to add options to allow line breaks in protocols/enums/class/structs. I would prefer this rule only for function bodies

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