-
Notifications
You must be signed in to change notification settings - Fork 287
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 use_air()
#2109
Add use_air()
#2109
Conversation
Should this also run air on the whole project? |
With I think this function is more in line with other one-time-setup helpers than with The current initial workflow for Air in Positron for an existing project looks like:
The current initial workflow for Air in RStudio for an existing project looks like:
I don't think we can improve on RStudio any further unless it bundles Air. But with VS Code / Positron I'd like to remove the CLI from the picture so it looks like:
I don't think having |
Ok, in that case, I'd recommend that you describe that in the docs. Or maybe include a todo bullet to remind the user to also run air format themselves. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be cool! I made a few comments, many of which surface existing patterns in usethis that could be used here.
R/air.R
Outdated
#' `.vscode/extensions.json`. This includes: | ||
#' | ||
#' - Setting `editor.formatOnSave = true` for R files to enable formatting on | ||
#' every save. Does nothing if `editor.formatOnSave` was already set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something of an existing convention where we use can_overwrite()
to determine if it's safe to err on the side of making change in a file. If the user has set the usethis.overwrite
option to TRUE
and we're making changes inside a project that is a Git repo, we just go ahead and do it. I have this set to TRUE
in my .Rprofile
.
I'm not sure if you'd like to do that here, but just wanted you to know about this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd use that if we were completely overwriting. In this particular case, we want to merge with what the user already has as much as possible, so I think not asking or checking is ok in this case.
I do think I'm going to remove this though
Does nothing if `editor.formatOnSave` was already set.
Does nothing if `editor.defaultFormatter` was already set.
I think we actually should update this, as you've explicitly requested Air with use_air()
, so you've opted into these recommended settings already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah when I first looked at this I thought maybe it was overly cautious / overly respectful of existing settings. Happy to have it just make these changes unconditionally.
…ssage when actually creating the file
…ally create them
The CI failures seem to be related to bad weather at httpbin.org and not related to the content of this PR. |
} | ||
|
||
write_air_vscode_extensions_json <- function(path) { | ||
settings <- jsonlite::read_json(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this operation may fail because settings.json
is actually JSONC (JSON with Comments), not JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Darn! jeroen/jsonlite#394
For posit-dev/air#224
Note that while
use_tidy_style()
actually invokes styler,use_air()
is just about getting your project set up to use Air. The documentation states everything that this does so I won't repeat here.I can't use
jsonlite::write_json()
directly due to some fiddly details I documented and talked to @jeroen about, so we have a small shim that mimics it for use with VS Code settings json files.I've tried to structure these functions with potential future use in mind.
I would not be surprised if writing JSON back to file doesn't result in a perfect roundtrip in terms of formatting, but I think that should be okay, especially if people use version control.