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

Add newline after multiline function declaration #371

Open
krlmlr opened this issue Mar 13, 2018 · 12 comments
Open

Add newline after multiline function declaration #371

krlmlr opened this issue Mar 13, 2018 · 12 comments

Comments

@krlmlr
Copy link
Member

krlmlr commented Mar 13, 2018

# Good
f1 <- function(a, b) {
  code
}

f2 <- function(a, b,
               c) {

  code
}

# Bad
f3 <- function() {

  code
}
@lorenzwalthert
Copy link
Collaborator

Is there a corresponding rule in the tidyverse style or do you plan to add one?

@krlmlr
Copy link
Member Author

krlmlr commented Mar 13, 2018

I remember Hadley suggested that an empty line after a long function header helps readability. I wouldn't formalize this as a rule yet, but I've seen this pattern a few times in dplyr.

@lorenzwalthert
Copy link
Collaborator

Ok. I think this is easy to implement. I just think for the sake of consistency, we should in general aim to keep the style guide in sync with styler.

@lorenzwalthert
Copy link
Collaborator

For a propper implementation, we'd need #357 because we want to add a line break dependent on whether a token is multi-line, which we cannot do if the multi-line attribute is not up-to date. The multi-line attribute is not up-to-date because it is only updated after all line break rules were applied.

@lorenzwalthert
Copy link
Collaborator

Blocked by #357.

@lorenzwalthert
Copy link
Collaborator

Implemented in mlr branch: lorenzwalthert@2a3fbc6

@lorenzwalthert
Copy link
Collaborator

The implementation in the mlr branch now depends on the number of lines in the body.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Apr 17, 2019

For a propper implementation, we'd need #357 because we want to add a line break dependent on whether a token is multi-line, which we cannot do if the multi-line attribute is not up-to date. The multi-line attribute is not up-to-date because it is only updated after all line break rules were applied.

Wrong. We need the exact line count, which we can compute ad-hoc within the transformer. It's as simple as in lorenzwalthert@d9f1b22. Performance implications probably small for non-package code because only taking effect for nests that are function declarations. Implementing it otherwise is most likely not much faster (i.e. doing it with a visitor for all top-level tokens) because most function declarations are not nested. (apparently there are more nested cases where this is insufficient, see #773). This rule needs to be at the end of the line-break transformers because it depends on them.

@maelle
Copy link
Contributor

maelle commented Oct 14, 2021

@krlmlr any specific examples of this pattern? 🙏

@maelle
Copy link
Contributor

maelle commented Oct 14, 2021

For context I was looking into never removing the newline at the beginning of a function body if there was one, following a comment by @mpadge. 😁

@krlmlr
Copy link
Member Author

krlmlr commented Oct 14, 2021

I have tweaked the "bad" example in the original post.

Keeping an extra newline, at least in non-strict mode, doesn't sound too bad as a starter.

@maelle
Copy link
Contributor

maelle commented Oct 14, 2021

I have tweaked the "bad" example in the original post.

That's what I had guessed it should be. 😁

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

No branches or pull requests

3 participants