Skip to content

Indention based on square brackets #207

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

Merged
merged 3 commits into from
Sep 19, 2017

Conversation

lorenzwalthert
Copy link
Collaborator

Closes #206. Note that in the wake of this PR, I merged indent_curly() and indent_round() to indent_braces() to reduce code duplication, which in addition, indents based on [ also.
Note that the indention is not token-dependent (=constant), so

abjdsl[abc,
2]

becomes

abjdsl[abc,
  2]

And not

abjdsl[abc,
       2]

Replace style_curly and style_round with style_brace that can also style square braces
@lorenzwalthert
Copy link
Collaborator Author

@dracodoc does that look better? At least for now, I think we can't easily support token-dependent indention unfortunately.

@lorenzwalthert
Copy link
Collaborator Author

@dracodoc you can also use the option scope = "spaces" to use the initial indentions in case you don't agree with the token-independent indention.

@dracodoc
Copy link

Indentation is a complex issue. I think (, [ should have different treatment from { in general. Usually people will want indentation for (, [ to align with them, with the exception that too many new lines are needed because of long expression or already near the end of 80 characters, in that case it's understandable to indent further on left side.

Of course you may want to prioritize your tasks for now, so I think it's OK for now.

@lorenzwalthert
Copy link
Collaborator Author

Right. Token-dependent indention is explicitly not encouraged in the style guide we aim to support, see section 2.5 in the tidyverse style guide, so that's why it was/is not of high priority. I note though that many people use it and I hope we can support it in the future.

@dracodoc
Copy link

The style guide 2.5 seemed to be more about keep code within 80 characters for me, not how much indentation. The "bad" example is bad not because it indent to token, but it's over 80 characters. For statement within 80 characters, this usage should be perfectly fine.

do_something("that", requires, many, arguments,
             "some of which may be long")

The tidyverse style guide didn't give details for indentation (ony {} is mentioned). Generally one would indent similar elements to same indentation, so both examples below should be fine, since the 1st one have all items moved to next line, and the 2nd one have other items aligned to former items. Then you may have to use 1st format if you are over the 80 characters limit.

do_something_very_complicated(
  "that",
  requires = many,
  arguments = "some of which may be long"
)

do_something("that", requires, many, arguments,
             "some of which may be long")

@lorenzwalthert
Copy link
Collaborator Author

Well, one might argue. But it says

If a function call is too long to fit on a single line, use one line each for the function name, each argument, and the closing )

To me, this is unambiguous (although the examples are not, I agree). So we just felt we are going to implement this in a strict way, which is also the easiest thing to do anyway.

I think this line of argument is supported when looking at the next example:

# Good
paste0(
  "Requirement: ", requires, "\n",
  "Result: ", result, "\n"
)

This call is much shorter but the line is still broken after (.

@dracodoc
Copy link

I read it again, yes you are right about the function call style. There is no mention about [], though we can use same rule. If you are to follow the same style for [], the example in beginning should be

abjdsl[
  abc,
  2
]

instead of this.

abjdsl[abc,
  2]

Right?

@lorenzwalthert
Copy link
Collaborator Author

Right, thanks. I have not thought about this much actually since I hardly ever put an expression between [ and ] that is longer than one line.
@krlmlr What do you think is preferred?

a[first_long_expression_here, 
  second_long_expression_down_nere]

Or

a[
  first_long_expression_here, 
  second_long_expression_down_nere
]

I think the latter is preferred since consistent with the other brace expression rules we have.

@krlmlr
Copy link
Member

krlmlr commented Sep 19, 2017

I also prefer the second variant.

@lorenzwalthert lorenzwalthert merged commit a029d4f into r-lib:master Sep 19, 2017
@lorenzwalthert lorenzwalthert deleted the indent_sq branch September 19, 2017 21:46
krlmlr added a commit that referenced this pull request Oct 23, 2017
- Hotfix: utf8 should not be verbose (#245).
- Allow styling of Rmd files(#233).
- Remove duplicate @family (#244).
- Fixing token insertion (#242).
- Capitalize Addin titles (#241).
- Explicit `NULL` creation to make styler compatible with R3.2.0 (#237).
- Improve vignettes (#232).
- Allow exclusion of files with `style_pkg()` and `style_dir()`.
- Correct styling with long strings (#230).
- Add tools for re-indenting tokens (#217).
- Math token spacing (#221).
- Remove outdated line and col information (#218).
- Empty input for styling does not cause an error (#227, #228).
- Tools to insert tokens + application on `if`-`else` clauses (#212).
- Improve example in documentation (#222).
- Fix spacing around in (#214).
- Maintenance: renaming functions / files, extend helper, documentation, if_else etc. (#204).
- Disallow line break after ( for function calls (#210).
- Preserve space between `!` and bang (#209).
- Simplify RStudio Addin (#208, #211).
- Indention based on square brackets (#207).
- Add vignette on introducing styler (#203).
- Indent function declaration without curly braces correctly (#202).
- Fix indention in if-else statement (#200).
- Sorting key (#196).
- Use safe sequences (#197).
- Fix space between two commas (#195).
- Keep single-line pipes on one line (#74).
- Remove tidyr and dplyr dependency (#182, #183, @jimhester).
- Fix parsing inconsistency (#188).
- Substitute create filler (#190).
- Introducing class vertical (#189).
- Adapt line break rules (#172).
- Fix `R CMD check` (#185).
- Force argument evaluation for proper error handling (#179).
- Add nonstrict version of set_space_before_comment (#174).
- Add installation instructions to README (#175).
- Addin to style highlighted region (#143).
- Improve spelling (#168).
- Add coverage badge
- Change badge from WIP to active
- Add the number of files to message (#166).
- Improve documentation (#164).
- Add informative messages while styling files (#165).
- More examples in help file (#161).
- No line breaks after pipe if comment is next token (#160).
- Fixing spacing around `!` (non-bang-bang) (#157).
- Finalize function documentation (#154).
- Review vignette (#158).
- Update bang-bang rule (#156).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants