Skip to content

Multiline function arguments #254

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

Closed
krlmlr opened this issue Oct 24, 2017 · 12 comments
Closed

Multiline function arguments #254

krlmlr opened this issue Oct 24, 2017 · 12 comments

Comments

@krlmlr
Copy link
Member

krlmlr commented Oct 24, 2017

styler::style_text("with(\na = 1,\n{\ncode\n}\n)", strict = FALSE, scope = "line_breaks")
#> with(
#>   a = 1, {
#>     code
#>   }
#> )

The opening brace should stay on its own line, we should also insert a line break if there is none.

styler::style_text("with(a = 1,\n{\ncode\n})", strict = FALSE, scope = "line_breaks")
#> with(a = 1, {
#>   code
#> })

The test_that() calls should be an exception: We allow this if the code argument is the last one and all other arguments start on the same line as the argument

styler::style_text("call({\ncode\n},\narg1 = FALSE)", strict = FALSE, scope = "line_breaks")
#> call({
#>   code
#> },
#> arg1 = FALSE)

In this case, I think we should add a line break after the opening paren even for strict = FALSE, unless the code block is the only argument.

styler::style_text("structure(list(\na=1), class=)")
#> structure(list(
#>   a = 1
#> ), class = )

Essentially the same as above. (And we never continue after a multi-line argument, the class = goes on its own line.)

@krlmlr krlmlr changed the title Code blocks as function arguments Multiline function arguments Oct 24, 2017
@lorenzwalthert
Copy link
Collaborator

Reference: #125.

@lorenzwalthert
Copy link
Collaborator

As far as the first example goes, I think the tidyverse does not currently encourage that in my opinion. It just says

A { should never go on its own line and should always be followed by a new line.

So I suggest to first file an issue there and see whether we can extend the style guide in this regard.

@krlmlr
Copy link
Member Author

krlmlr commented Oct 31, 2017

Oh, I see.

@lorenzwalthert
Copy link
Collaborator

@krlmlr I was just going to label issues we won't solve soonish with Status: Postponed and then closing them. Are you ok with that strategy or should we leave all unsolved issues open?

@krlmlr
Copy link
Member Author

krlmlr commented Nov 3, 2017

The label + closing feels like a good idea. Do you have a writeup that describes the labeling strategy?

@lorenzwalthert
Copy link
Collaborator

Ok, cool. No I don't have a write up, but we could add one. I actually checked out this but then I felt like it's not exactly what we need, so I just adapted it to what we have now. Maybe I can even briefly describe it in a contribution guide following the GitHub Contribution Guidelines?

@krlmlr
Copy link
Member Author

krlmlr commented Nov 3, 2017

We could start with a link to the blog and a description of your changes. Only collaborators can add/change labels, not sure how useful it is to have this info in the contribution guidelines?

@lorenzwalthert
Copy link
Collaborator

Ok, if you like. I thought it may be useful for non-collaborators just to see what the labels mean and how they are used.

@krlmlr
Copy link
Member Author

krlmlr commented Nov 3, 2017

That level of information feels useful, maybe a paragraph with a link to the full text?

@lorenzwalthert
Copy link
Collaborator

Ok, I will first create the text that describes what labels we have and how I adapted from the blog and then we can see how we proceed. Is not gonna fill books anyways 😄.

@martin-mfg
Copy link

Now that the discussed pull request tidyverse/style/pull/46 filed by @krlmlr got merged and thus the style guidelines for { and } have changed a bit, I think this issue could be reopened. (And I'm looking forward to this issue being resolved. 😃)
And furthermore I think the pull request being merged is subsequently also relevant for #125.

@lorenzwalthert
Copy link
Collaborator

Thanks @martin-mfg for following up on that. As outlined by saamwerk, which guides the labelling for this repo, all issues that are postponed are closed. I elaborated a bit on the description for "Status: Postponed" in lorenzwalthert/saamwerk@6cbb797 just now to make the meaning clearer. Although the issue gains in relevance with the merge of tidyverse/style#46, I don't think I can resolve this soonish (or with styler 1.0.1, see #341) , so I prefer to leave it closed for now. If you want to contribute via a PR, I am happy to review. However, changes are that we can resolve quite a few postponed issues between June and August.

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