For long function declarations, add newline before return arrow as well #17

teradyl · 2018-09-10T20:14:26Z

Summary

Function declarations sometimes have long unwieldy lines with the return type. This can fix that problem.

This is the updated version of: #15

Reasoning

Stay within the 100 character limit. By doing this on ALL long function declarations, we keep some consistency with where to find the return type.

Clarification: One-line functions with return signatures that are under the 100 character limit should still be in one line.

Reviewers

cc @airbnb/swift-styleguide-maintainers

Please react with 👍/👎 if you agree or disagree with this proposal.

fdiaz · 2018-09-11T11:08:56Z

README.md

@@ -334,7 +334,7 @@ _You can enable the following settings in Xcode by running [this script](resourc

  </details>

-* <a id='long-function-declaration'></a>(<a href='#long-function-declaration'>link</a>) **Separate [long](#column-width) function declarations with line breaks before each argument label.** Put the open curly brace on the next line so the first executable line doesn't look like it's another parameter. SwiftLint: [`multiline_parameters`](https://github.com/realm/SwiftLint/blob/master/Rules.md#multiline-parameters), [`vertical_parameter_alignment_on_call`](https://github.com/realm/SwiftLint/blob/master/Rules.md#vertical-parameter-alignment-on-call)
+* <a id='long-function-declaration'></a>(<a href='#long-function-declaration'>link</a>) **Separate [long](#column-width) function declarations with line breaks before each argument label and before return arrow.** Put the open curly brace on the next line so the first executable line doesn't look like it's another parameter. SwiftLint: [`multiline_parameters`](https://github.com/realm/SwiftLint/blob/master/Rules.md#multiline-parameters), [`vertical_parameter_alignment_on_call`](https://github.com/realm/SwiftLint/blob/master/Rules.md#vertical-parameter-alignment-on-call)


Should this be "and before the return arrow"?

thedrick · 2018-09-11T17:34:50Z

Does the vertical_parameter_alignment_on_call swift lint rule actually fix this completely? My guess is that it won't change

func generateStars(
  at location: Point,
  count: Int,
  color: StarColor,
  withAverageDistance averageDistance: Float) -> String
{
  populateUniverse()
}

to

func generateStars(
  at location: Point,
  count: Int,
  color: StarColor,
  withAverageDistance averageDistance: Float) 
  -> String
{
  populateUniverse()
}

which I think makes this a little confusing

fdiaz · 2018-09-11T22:04:24Z

@thedrick It won't. This PR isn't adding any SwiftLint rules. Both of those examples are valid for the linter right now.

thedrick · 2018-09-11T22:43:08Z

@fdiaz what is our policy on rules like this that aren't easily corrected by the linter? The Airbnb codebase doesn't currently follow this proposal and it would take a lot of work to manually update it to do so.

fdiaz · 2018-09-11T22:55:25Z

what is our policy on rules like this that aren't easily corrected by the linter? The Airbnb codebase doesn't currently follow this proposal and it would take a lot of work to manually update it to do so.

I don't think we have one. I'd be extra cautious to accept changes that aren't currently adopted in our codebase without consulting with the team (which is what I'll be doing this Friday on the internal newsletter).

I'd hope more people would comment after that, but in general for breaking changes like this I'd expect a bigger acceptance rate to approve than for something we already use in our codebase.

Does that make sense?

thedrick · 2018-09-11T22:57:37Z

Yeah that makes sense to me. I think I actually like this proposal, but I worry it won't be feasible for us to adopt it in our codebase wholesale which will leave us in a weird state where some of the code uses this new style and most of our code uses the old style. If we had a script or lint rule that could auto-correct this for everything I'd be more on board with this addition.

bachand · 2018-09-12T04:12:00Z

That's a good question @thedrick.

@teradyl do you think we could add a migration plan to the PR message? What do you think @fdiaz ?

bachand · 2018-09-12T04:29:52Z

I was trying to do this with sed via find, but ran into trouble with the newlines. Here's where I got (this is the identity function, making no changes).

find . -type f -name "*.swift" -exec sed -i '' 's/\(func.*\)->/\1->/g' {} \;

fdiaz · 2018-09-12T18:59:22Z

do you think we could add a migration plan to the PR message?

I would advice against it. This style guide should be decoupled from our own codebase IMO. But I'd think it's also reasonable to require an autofix SwiftLint rule for this too given this is not our current adopted pattern at all (as in, this will need to change all usages).

teradyl · 2018-09-12T20:58:39Z

On the one hand, I understand that we don't want to accept a style guide rule that we don't have applied and implemented yet. On the other hand, it's hard to proceed with a better style if we don't have anything in the style guide that guides us towards a better future.

If someone wants to create an autofix SwiftLint rule for this, that would solve the problem, but I think that's a non-trivial amount of work, and if no one wants to do that we'll be stuck in limbo with old style.

I would like to see rules like this go through (if everyone indeed agrees it's a good style idea) so new and updated code can in fact be written to a better style. Our code already has violations of the style guide that are slowly updated, and this will just be another (albeit more common at first) instance of that.

I would urge people to rate this rule notwithstanding the current state of our codebase.

thedrick · 2018-09-13T00:02:14Z

@teradyl well put, you've convinced me and I've changed my 👎to a 👍:)

swiftal64 · 2018-09-14T22:23:57Z

@teradyl could we collaborate on the regex that needs to be created to fix all instances?

I started one here: ~~https://regex101.com/r/h7qzfy/1~~ https://regex101.com/r/h7qzfy/2

If anyone wants to add complex examples to that regex101, we could work on this regex together

teradyl · 2018-09-17T23:21:01Z

@swift-ortal Thanks for making that regex! Looks good. I think the more annoying part is integrating it into SwiftLint.

You did however remind me of the throws keyword that can be there, so I added examples with that.

@fdiaz We can merge this on Wednesday correct?

I can send out a PSA for people to start working this style into their workflows.

fdiaz · 2018-09-18T17:19:56Z

@fdiaz We can merge this on Wednesday correct?

I'm planning on approving this tomorrow but leave it open until we can add a SwiftLint rule so we can lint for it.

SwiftLint does allow regex so if we're comfortable with that regex we can just use that but it'd be cool if we can also add a SwiftLint rule so we can contribute back to the community and it would be easier for people that want to adopt the same rule.

fdiaz · 2018-09-20T21:12:46Z

Resolution

Status: Approved ✅ / Waiting for SwiftLint rule to be merged.

Reasoning:

This had an overwhelming positive reaction from the team. There were some concerns regarding linting that should be solved by providing a SwiftLint rule before we merge but this is approved.

swiftal64 · 2018-09-22T22:53:00Z

README.md

+      color: StarColor,
+      withAverageDistance averageDistance: Float) throws
+      -> String {
+      populateUniverse() // this line blends in with the argument list


You can delete the comment // this line blends in with the argument list on this line

I think that comment still makes sense. We still want to make sure that we put the open curly brace on the next line.

swiftal64 · 2018-09-22T22:54:07Z

README.md

@@ -334,7 +334,7 @@ _You can enable the following settings in Xcode by running [this script](resourc

  </details>

-* <a id='long-function-declaration'></a>(<a href='#long-function-declaration'>link</a>) **Separate [long](#column-width) function declarations with line breaks before each argument label.** Put the open curly brace on the next line so the first executable line doesn't look like it's another parameter. SwiftLint: [`multiline_parameters`](https://github.com/realm/SwiftLint/blob/master/Rules.md#multiline-parameters), [`vertical_parameter_alignment_on_call`](https://github.com/realm/SwiftLint/blob/master/Rules.md#vertical-parameter-alignment-on-call)
+* <a id='long-function-declaration'></a>(<a href='#long-function-declaration'>link</a>) **Separate [long](#column-width) function declarations with line breaks before each argument label and before the return signature.** Put the open curly brace on the next line so the first executable line doesn't look like it's another parameter. SwiftLint: [`multiline_parameters`](https://github.com/realm/SwiftLint/blob/master/Rules.md#multiline-parameters), [`vertical_parameter_alignment_on_call`](https://github.com/realm/SwiftLint/blob/master/Rules.md#vertical-parameter-alignment-on-call)


We'll want to convert these swiftlint links to shields.io badges that the style guide now uses

fdiaz · 2018-10-10T15:53:29Z

Just rebased to resolve conflicts and move this branch forward. Whenever the regex is ready we can add it to our swiftlint.yml file as a custom rule and then we can merge this.

cc @teradyl @swift-ortal

teradyl · 2018-10-11T22:07:29Z

Thanks @swift-ortal for helping me with the regex. I rebased and put the regex in the swiftlint. Please take a look.

dfed · 2018-10-11T22:39:27Z

resources/swiftlint.yml

+
+  long_function_return_arrow_newline:
+    name: "long_function_return_arrow_newline"
+    regex: (\bfunc [^(]+\(\n[^)]+?\)) *(throws\b|->)


Any way we could comment this regex? This will be hard to maintain as-is.

Also, I'm curious: how does this only affect long lines?

@dfed It's easier to just direct you at the regex link where Ortal & I created it so you can see for yourself the examples where it triggers and not, along with explanations for each part of the regex. https://regex101.com/r/h7qzfy/3

I updated the message in swiftlint to include that link.

Will check it out later today. ~~In the meantime, could we link to that site in a comment? Keeping that info alive will help future maintainers.~~

Edit: You said you already did that! Thanks 😄. Not sure how I missed that.

In the future I wonder if we should put the regex link as a code comment. Seems useful for maintainers.

@bachand It is added in the message https://github.com/airbnb/swift/pull/17/files#diff-7a1b543d4d8e0f3369a51e6e049c47d7R52 😉

Nice. Sorry I missed that

fdiaz · 2018-10-19T17:34:21Z

Resolution

Status: Approved ✅

Reasoning:

This had an overwhelming positive reaction from the team.

teradyl mentioned this pull request Sep 10, 2018

Added rule for long lines with a long return value #15

Closed

fdiaz reviewed Sep 11, 2018

View reviewed changes

swiftal64 reviewed Sep 22, 2018

View reviewed changes

fdiaz force-pushed the dkh--newline-for-return-value-on-function branch from ce89a64 to a0b8dc0 Compare October 10, 2018 15:52

teradyl added 4 commits October 11, 2018 14:52

added newline before return arrow for long function declarations

1dc4d04

added "the"

a9b4563

added examples of long functions with throws

420bd2c

add swiftlint regex

57f0185

teradyl force-pushed the dkh--newline-for-return-value-on-function branch from a0b8dc0 to 57f0185 Compare October 11, 2018 22:05

dfed reviewed Oct 11, 2018

View reviewed changes

added link for more info on regex

3eff078

fdiaz approved these changes Oct 19, 2018

View reviewed changes

fdiaz merged commit ee9ec10 into master Oct 19, 2018

fdiaz deleted the dkh--newline-for-return-value-on-function branch October 19, 2018 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For long function declarations, add newline before return arrow as well #17

For long function declarations, add newline before return arrow as well #17

teradyl commented Sep 10, 2018 •

edited

fdiaz Sep 11, 2018 •

edited

thedrick commented Sep 11, 2018

fdiaz commented Sep 11, 2018

thedrick commented Sep 11, 2018

fdiaz commented Sep 11, 2018

thedrick commented Sep 11, 2018

bachand commented Sep 12, 2018 •

edited

bachand commented Sep 12, 2018

fdiaz commented Sep 12, 2018

teradyl commented Sep 12, 2018

thedrick commented Sep 13, 2018

swiftal64 commented Sep 14, 2018 •

edited

teradyl commented Sep 17, 2018

fdiaz commented Sep 18, 2018

fdiaz commented Sep 20, 2018

swiftal64 Sep 22, 2018

teradyl Oct 2, 2018

swiftal64 Sep 22, 2018

fdiaz commented Oct 10, 2018

teradyl commented Oct 11, 2018 •

edited

dfed Oct 11, 2018

teradyl Oct 17, 2018

dfed Oct 19, 2018 •

edited

bachand Oct 23, 2018

fdiaz Oct 24, 2018

bachand Oct 25, 2018

bachand Oct 25, 2018

fdiaz commented Oct 19, 2018

For long function declarations, add newline before return arrow as well #17

For long function declarations, add newline before return arrow as well #17

Conversation

teradyl commented Sep 10, 2018 • edited

Summary

Reasoning

Reviewers

fdiaz Sep 11, 2018 • edited

Choose a reason for hiding this comment

thedrick commented Sep 11, 2018

fdiaz commented Sep 11, 2018

thedrick commented Sep 11, 2018

fdiaz commented Sep 11, 2018

thedrick commented Sep 11, 2018

bachand commented Sep 12, 2018 • edited

bachand commented Sep 12, 2018

fdiaz commented Sep 12, 2018

teradyl commented Sep 12, 2018

thedrick commented Sep 13, 2018

swiftal64 commented Sep 14, 2018 • edited

teradyl commented Sep 17, 2018

fdiaz commented Sep 18, 2018

fdiaz commented Sep 20, 2018

Resolution

Status: Approved ✅ / Waiting for SwiftLint rule to be merged.

Reasoning:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdiaz commented Oct 10, 2018

teradyl commented Oct 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfed Oct 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdiaz commented Oct 19, 2018

Resolution

Status: Approved ✅

Reasoning:

teradyl commented Sep 10, 2018 •

edited

fdiaz Sep 11, 2018 •

edited

bachand commented Sep 12, 2018 •

edited

swiftal64 commented Sep 14, 2018 •

edited

teradyl commented Oct 11, 2018 •

edited

dfed Oct 19, 2018 •

edited