-
Notifications
You must be signed in to change notification settings - Fork 14
Implement if statement autobracing #340
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
Open
DavisVaughan
wants to merge
15
commits into
main
Choose a base branch
from
feature/if-else-bracing
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…fs in `consequence` They should get to compute their own value, because the user could have validly written a one liner if statement inside a braced if to begin with, and we want to be consistent with that.
This causes too many issues with idempotence and verbatim handling, even though it would be quite nice
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #334
Closes #225
This PR also has the side effect of fixing all idempotence issues on both data.table and R core. Those projects probably won't ever use Air, but they are a very good test case for "are we stable between runs?". We should probably set up some CI for ourselves that runs some kind of idempotence check on a set of large community packages (dplyr, data.table, r-svn) to ensure we don't slip here.
This PR introduces the idea of autobracing. This is the idea of wrapping an if statement body in
{ }
This ensures that the if statement is portable. The above original if statement actually won't even parse at top level due to the
else
on its own line. If you wrap the whole thing in{ }
(like would be the case if you found it inside a function body) then it parses fine. Portability ensures:We still allow one liner if statements in a few specific contexts:
<-
,=
,<<-
)In addition to being in that context, the one liner also has to be "simple enough" for some definition of "simple" which is encoded in
compute_braced_expressions()
in the PR.I think the formatter code for this feature is rather elegant and easy enough to follow. By far the most complex part of this PR is comment handling. I've added a huge slew of comment related tests to ensure we are doing it "well enough". Movement of comments with something like this will always be a "best effort" kind of transformation, but I think it is worth a little ambiguity. The main promise we make is that we won't ever drop your comments.