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

ARROW-13562: [R] Styler followups #10879

Closed
wants to merge 6 commits into from

Conversation

nealrichardson
Copy link
Member

Adds styling tasks to the Makefile (for 🦖 like me; I found that the styling-on-save from vscode was not reliable). Also makes codegen.R generate styled R code.

@nealrichardson nealrichardson requested a review from jonkeane August 4, 2021 19:15
@github-actions
Copy link

github-actions bot commented Aug 4, 2021

r/Makefile Outdated
R -s -e 'setwd(".."); if (requireNamespace("styler")) styler::style_file(setdiff(system("git diff --name-only | grep r/.*R$$", intern = TRUE), c("r/tests/testthat/latin1.R", "r/data-raw/codegen.R")))'

style-all:
R -s -e 'styler::style_file(setdiff(dir(pattern = "R$$", recursive = TRUE), c("tests/testthat/latin1.R", "data-raw/codegen.R")))'
Copy link
Member

Choose a reason for hiding this comment

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

We don't have to do this here, but given all where we are putting them, I wonder if we want to make (or investigate if this exists in styler already...) a file in r/arrow that specifies which files are no-style so we only have one place we need it

Copy link
Member

Choose a reason for hiding this comment

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

Turns out styler doesn't have this, though they are considering ways to do it. r-lib/styler#319

We could add our own and "just" source that file that lists the files we want to skip so that we only have to list them once.

@jonkeane jonkeane closed this in cbcf5cb Aug 4, 2021
@lorenzwalthert
Copy link
Contributor

Adds styling tasks to the Makefile (for 🦖 like me; I found that the styling-on-save from vscode was not reliable). Also makes codegen.R generate styled R code.

Hi @nealrichardson, can you maybe tell us a bit more about problems with that so we could improve that?

@nealrichardson
Copy link
Member Author

Adds styling tasks to the Makefile (for 🦖 like me; I found that the styling-on-save from vscode was not reliable). Also makes codegen.R generate styled R code.

Hi @nealrichardson, can you maybe tell us a bit more about problems with that so we could improve that?

@lorenzwalthert the first thing I found was that, while style-on-save would clearly do something (like remove extra whitespace), I was still getting lint warnings about files not having a trailing newline. I didn't get this if I just ran styler directly on the file. Later, it seemed like the style-on-save wasn't firing at all. I figured it was possible that either of those could just be user error, and I wanted a simple (to me) way to re-style all of the files anyway, so I added these make commands. If it would be helpful, I'm happy to write an issue somewhere.

@lorenzwalthert
Copy link
Contributor

Thanks @nealrichardson. I referenced your comment in an open issue (REditorSupport/languageserver#462), I hope this gets resolved. Regarding that formatting wasn't fired at all, I am unsure what the problem was. Just noted several issues in the above repo where the problem was the fact that styler is quite slow on unseen code (r-lib/styler#558).

michalursa pushed a commit to michalursa/arrow that referenced this pull request Aug 17, 2021
Adds styling tasks to the Makefile (for 🦖  like me; I found that the styling-on-save from vscode was not reliable). Also makes codegen.R generate styled R code.

Closes apache#10879 from nealrichardson/styler2

Lead-authored-by: Jonathan Keane <jkeane@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimport asfimport mentioned this pull request Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants