Avoid squeezing reaction, method, or preamble bodies onto a single line #1984
Conversation
This file contains 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
edwardalee
reviewed
Sep 1, 2023
There was a problem hiding this comment.
I think this change is a good one, but it doesn't solve the problem for TypeScript, since semicolons are optional. Would it be possible for the formatter to simply accept whatever the user chooses in this regard? I still think we should reformat all our examples and tests to not inline single-line reactions, however. I've been doing that manually for all the examples in the Handbook (please do not run the formatter on them!!).
petervdonovan
force-pushed
the
update-formatter
branch
from
6bcb756
to
90967d4
Compare
petervdonovan
changed the title
|
I undid the changes that I had originally implemented and changed to a completely different implementation that I think is less kludgy. I think this PR is ready now. |
petervdonovan
changed the title
petervdonovan
changed the title
petervdonovan
force-pushed
the
update-formatter
branch
2 times, most recently
from
4bfcbb3
to
3baaf1d
Compare
edwardalee
approved these changes
Sep 3, 2023
petervdonovan
force-pushed
the
update-formatter
branch
3 times, most recently
from
c3846f0
to
d920e24
Compare
This adds state, but it is a private non-static member and the formatter is not parallelized, so it should be OK. The technique employed here is I think the most reasonable way to account for context-sensitive formatting rules.
The comment about thread-safety in the previous commit message was incorrect because I forget that we were using the singleton pattern on ToLf. To my knowledge there was no good reason for us to do that.
This preamble was not even appearing in the generated code, which causes LSP test to fail.
Reactions, preambles, and methods typically consist of multiple statements appearing on distinct lines, and if they consist of only a single statement, they commonly evolve into multiple statements. Therefore these three constructs should all use multiline code blocks.
petervdonovan
force-pushed
the
update-formatter
branch
from
d920e24
to
4b92a34
Compare
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.
Fixes #1975.
This is in response to readability issues. Additionally, I personally find that when statements are put on one line, that makes refactoring more cumbersome because one must add newlines before adding additional statements to a reaction.
An example of a project that made a similar decision to this is
rustfmt, which never seems to put if statements all on one line.This change has a code smell -- it does matching on raw strings without parsing them. Also it only has effects in the case where the target language has semicolons, which may or may not be what we want. I am thinking about changing the implementation of this to be based on whether the code is a reaction body instead of whether it has a semicolon.This is now resolved.Since this change is late, we do not need to squeeze it into the 0.5.0 release. However, it might behoove us to release it soon, e.g. in 0.5.1, because the changes to the appearance of LF code could be highly disruptive; ideally users could transition straight from the 0.4.0 format to the 0.5.1 format without bothering with the 0.5.0 format in between.