Add simple newline insertion to formatter #334
Conversation
This fixes "bracebug" (reported here: google#299) and many variants of it. Of course it's not a complete solution to any newline problems. In particular it never removes or rearranges newlines, it only adds some. But it still should help a lot.
|
Some pretty big test cases with a lot of examples are committed. Please take a look at them to check if the output style is acceptable. The code in FixNewlines may look a little bit repetitive, but it's hard to avoid that. A very simple task is performed on many AST types. Some of it could be abstracted away, e. g. by creating a function that returns "interesting" fodder objects for each. But I think we would still end up with similar number of functions and much less obvious code that requires some dancing around special cases. There are also some improvements to test scripts:
|
test_suite/tests.source
Outdated
| get_temp_dir() { | ||
| if [ -z "$TMP_DIR" ]; then | ||
| # Simplest and most portable way I could find. | ||
| TMP_DIR="`python2 -c "import tempfile; print(tempfile.mkdtemp(prefix='jsonnet_'))"`" |
There was a problem hiding this comment.
Did you consider something along the lines of
TMP_DIR=$(mktemp -d jsonnet_XXXX)
There was a problem hiding this comment.
Yes. There are portability problems. OS X and GNU versions of mktemp need different arguments. Invoking Python seems much cleaner to me than trying to autodetect the version or trying the other version if it fails (the latter is the top answer here https://unix.stackexchange.com/questions/30091/fix-or-alternative-for-mktemp-in-os-x).
core/formatter.cpp
Outdated
| // very_long_function_on_a_long_line_so_arguments_dont_fit_on_the_same_line( | ||
| // 1, 2, 3) | ||
| // Should be expanded to: | ||
| // very_long_function_on_a_long_line_so_arguments_dont_fit_on_the_same_line( |
There was a problem hiding this comment.
I'm not familiar with the other formatting passes, but are any considerations taken for line length? Reading the code here it seems the expansion happens when args are already on another line, not if function is too long.
There was a problem hiding this comment.
No, it doesn't take the length into account. The name was only an example situation when someone would format it this way (as opposed to keeping arguments on the same line). Hmmm... but I can see how it can be confusing. Maybe it's better to change it to something generic.
| // Should be expanded to: | ||
| // f(1, | ||
| // 2, | ||
| // 3) |
There was a problem hiding this comment.
Shouldn't the closing paren live on a new line to match the below style? Or is that handled separately?
There was a problem hiding this comment.
The point of this comment was to explain what this particular function does, expandBetween as opposed to expandNearParens (both operate on ArgParams).
But still no, it wouldn't be touched by the other one (it would if there was a newline before 1 or after 3).
| collCorrect2c: [ | ||
| { | ||
| name: "Bad", | ||
| }, self.someOther(),], |
There was a problem hiding this comment.
looks like 2c is the same as 2b
core/formatter.cpp
Outdated
| /// 'a': 'b', | ||
| /// 'c': 'd', | ||
| /// }] | ||
| /// The outer array can be unexpanded, because there are no newlines between |
core/formatter.cpp
Outdated
| /// | ||
| /// The main principle is that a structure can either be: | ||
| /// * expanded and contain newlines in all the designated places | ||
| /// * unexpanded and not contain any newlines in any of designated places. |
There was a problem hiding this comment.
Slightly more concise description:
/// * unexpanded and contain newlines in none of the designated places.
This fixes "bracebug" (reported here: #299) and many
variants of it.
Of course it's not a complete solution to any newline problems.
In particular it never removes or rearranges newlines, it only
adds some. But it still should help a lot.