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

Recommend single indent, not double indent #223

Merged
merged 4 commits into from
Oct 2, 2024
Merged

Recommend single indent, not double indent #223

merged 4 commits into from
Oct 2, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 30, 2024

Fixes #215

Does this correctly capture what you said in the discussion?

functions.Rmd Outdated

Prefer function-indent style to double-indent style when it fits.
Prefer function-indent style to single-indent style when it fits.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be one of the few formatting settings we would implement in Ark for legacy formatting: an option between function-style if it fits (legacy), or single-indent style everywhere ("modern"). Another very similar option would be call-indent style versus single-indent style for function calls, the former matching the RStudio default setting and the latter matching the tidyverse style.

But if we're bold enough, I now think the default / enforced style should be single-indent style to match JS/TS and Rust styling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea so the "legacy" formatter option logic would be:

  • Legacy
    • If it fits, don't break the arguments over multiple lines at all, else
    • If it fits, use function-style line breaks, else
    • Fall back to single-indent line breaks
  • Modern
    • If it fits, don't break the arguments over multiple lines at all, else
    • Fall back to single-indent line breaks

I think that Modern (or whatever we call it) would probably result in less code diffs in the long term, at the cost of one big set of upfront diffs as it changes existing function-style -> single-indent style everywhere. I think I'll probably end up preferring this.

Maybe the style guide should just not set a preference between these two styles right now? We can add a preference back in as we feel out the formatter.

functions.Rmd Outdated Show resolved Hide resolved
functions.Rmd Outdated Show resolved Hide resolved
functions.Rmd Outdated

Prefer function-indent style to double-indent style when it fits.
Prefer function-indent style to single-indent style when it fits.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea so the "legacy" formatter option logic would be:

  • Legacy
    • If it fits, don't break the arguments over multiple lines at all, else
    • If it fits, use function-style line breaks, else
    • Fall back to single-indent line breaks
  • Modern
    • If it fits, don't break the arguments over multiple lines at all, else
    • Fall back to single-indent line breaks

I think that Modern (or whatever we call it) would probably result in less code diffs in the long term, at the cost of one big set of upfront diffs as it changes existing function-style -> single-indent style everywhere. I think I'll probably end up preferring this.

Maybe the style guide should just not set a preference between these two styles right now? We can add a preference back in as we feel out the formatter.

@hadley
Copy link
Member Author

hadley commented Oct 1, 2024

It's great that you prefer the single-indent style now that I've finally shifted to liking the function indent style.

@hadley
Copy link
Member Author

hadley commented Oct 1, 2024

It's worth another look now. I also added some advice on S7.

functions.Rmd Outdated
# As usual code is indented by two spaces.
}
```

* Double-indent: Place each argument of its own **double** indented line.
* **Function-indent**: indent the argument name to match the opening `(` of `function`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW we use the term hanging_indent_style in {lintr}:

https://lintr.r-lib.org/reference/indentation_linter.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah I like that better.

functions.Rmd Outdated

There are two options if the function name and definition can't fit on a single line:
There are two options if the function name and definition can't fit on a single line. In both cases, each argument goes on its own; the difference is how deep you indent it and where you put `)` and `{`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
There are two options if the function name and definition can't fit on a single line. In both cases, each argument goes on its own; the difference is how deep you indent it and where you put `)` and `{`:
There are two options if the function name and definition can't fit on a single line. In both cases, each argument goes on its own line; the difference is how deep you indent it and where you put `)` and `{`:

functions.Rmd Outdated
a = "a long argument",
b = "another argument",
c = "another long argument") {
# Bad
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to add # Bad? It's currently just another option right? (Same with # Good above)?

i.e. while i feel like we are starting to prefer single-indent, im not sure we are all in on it enough for the style guide to recommend that function-indent is # Bad

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, no I meant to write good.

@hadley hadley merged commit bf5bb29 into main Oct 2, 2024
1 check passed
@hadley hadley deleted the single-indent branch October 2, 2024 12:30
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.

Multiline function declarations
4 participants