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

Multiline lists are formatted wrong #307

Closed
DartBot opened this issue Jun 5, 2015 · 18 comments
Closed

Multiline lists are formatted wrong #307

DartBot opened this issue Jun 5, 2015 · 18 comments
Assignees

Comments

@DartBot
Copy link

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="96" height="96"hspace="10"> Issue by munificent
Originally opened as dart-lang/sdk#16381


Source:

main() {
  [
    "this is a very long string that forces the list to wrap",
    "this is a very long string that forces the list to wrap"
  ];
}

Output:

main() {
  ["this is a very long string that forces the list to wrap",
      "this is a very long string that forces the list to wrap"];
}

Expected: It should stay formatted like the source.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


Added this to the 1.2 milestone.
Removed Priority-Unassigned label.
Added Priority-Medium label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


What's the rule for lists? Which of these are acceptable/advisable?

var a = [1, 2, 3];
var b = [
  1,
  2,
  3
];
var c = [1, 2,
    3];
var d = [1, 2,
  3];


Set owner to @munificent.
Added NeedsInfo label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent


a and b are acceptable, and I think it's up to the user to choose which one they want. c and d are always wrong.

I would probably have the formatter:

  1. Control formatting after the "[".
  2. Control indentation of every line in the list.
  3. Control the formatting before the "]".
  4. Preserve the users newlines after ",".

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


What about?

var pairs = [
  1, 2,
  3, 4,
  5, 6
];

As a user I'd be frustrated if this got flattened.

On the other hand, what should happen if I add another pair?

var pairs = [
  1, 2,
  3, 4,
  5, 6, 7, 8
]
  
Obviously we can't get into the business of trying to divine the semantic meaning of user-introduced whitespace.

And what about long lists? Is there an acceptable version that does not have one item per line?

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/648527?v=3" align="left" width="48" height="48"hspace="10"> Comment by Fox32


I have a list defining a coordiantes for a cube with normals a texture coordinates. I want to use the formatter but the formatter will never be able to output it in the way I want it. Maybe it is better if the formatter don't touch the contents of the list. Then the user needs to do it on his own.

Here is the example for my list. I added additional whitespaces to make it easier to read (every number is lining up, even if it negativ) and to seperate the position from the normals and texture coordinates.

[
  [-0.5, -0.5, 0.5, 0.0, 0.0, -1.0, 0.0, 0.0,
    0.5, -0.5, 0.5, 0.0, 0.0, -1.0, 1.0, 0.0,
    0.5, 0.5, 0.5, 0.0, 0.0, -1.0, 1.0, 1.0,
   -0.5, 0.5, 0.5, 0.0, 0.0, -1.0, 0.0, 1.0],
  [-0.5, -0.5, 0.5, 1.0, 0.0, 0.0, 1.0, 1.0,
   -0.5, 0.5, 0.5, 1.0, 0.0, 0.0, 0.0, 1.0,
   -0.5, 0.5, -0.5, 1.0, 0.0, 0.0, 0.0, 0.0,
   -0.5, -0.5, -0.5, 1.0, 0.0, 0.0, 1.0, 0.0],
  [-0.5, 0.5, 0.5, 0.0, -1.0, 0.0, 1.0, 1.0,
    0.5, 0.5, 0.5, 0.0, -1.0, 0.0, 0.0, 1.0,
    0.5, 0.5, -0.5, 0.0, -1.0, 0.0, 0.0, 0.0,
   -0.5, 0.5, -0.5, 0.0, -1.0, 0.0, 1.0, 0.0],
  [ 0.5, 0.5, 0.5, -1.0, 0.0, 0.0, 1.0, 1.0,
    0.5, -0.5, 0.5, -1.0, 0.0, 0.0, 0.0, 1.0,
    0.5, -0.5, -0.5, -1.0, 0.0, 0.0, 0.0, 0.0,
    0.5, 0.5, -0.5, -1.0, 0.0, 0.0, 1.0, 0.0],
  [ 0.5, -0.5, 0.5, 0.0, 1.0, 0.0, 1.0, 1.0,
   -0.5, -0.5, 0.5, 0.0, 1.0, 0.0, 0.0, 1.0,
   -0.5, -0.5, -0.5, 0.0, 1.0, 0.0, 0.0, 0.0,
    0.5, -0.5, -0.5, 0.0, 1.0, 0.0, 1.0, 0.0]
],

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


Great example. Thanks for chiming in!

This is exactly why I'm leery of the formatter being too aggressive with lists.

A couple of off the cuff thoughts:

a) The formatter could ignore lists of this format:

l = [
 <contents here -- possibly on multiple lines>
];

assuming that if the author put contents on a separate newline they know what they're doing.

b) we could condition authors to use a doc convention to ensure their structure is left intact:

l = [
  [-0.5, -0.5, 0.5, 0.0, 0.0, -1.0, 0.0, 0.0,
     0.5, -0.5, 0.5, 0.0, 0.0, -1.0, 1.0, 0.0,
     0.5, 0.5, 0.5, 0.0, 0.0, -1.0, 1.0, 1.0,
   -0.5, 0.5, 0.5, 0.0, 0.0, -1.0, 0.0, 1.0],
...

(This will look familiar to eclipse Java users.)

I'm not in love with (b) but I'm curious what your gut reaction is...

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/648527?v=3" align="left" width="48" height="48"hspace="10"> Comment by Fox32


I don't like version b), I think the assumption that the user knows what he does, if he adds spaces, is a good way.

I first liked the idea 3. from comment #­3, but this wouldn't work with the minus sign formatting thing in my example.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent


What about?

var pairs = [
1, 2,
3, 4,
5, 6
];

As a user I'd be frustrated if this got flattened.

Me too. Hence rule 4 above.

On the other hand, what should happen if I add another pair?

I'd preserve that formatting too, unless it goes over 80 columns.
 

And what about long lists? Is there an acceptable version that does not have one item per line?

Sure, you can have long inline lists if you want, as long as the fit 80 columns. Once they go over, you need a newline after the "[" and before the "]".

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


Sure, you can have long inline lists if you want, as long as the fit 80 columns. Once they go over, you need a
newline after the "[" and before the "]".

Cool. Am I right that this list:

var primes = [2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 87];

would be reformatted to:

(A)

var primes = [
  2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59, 61, 67, 71,
  73, 79, 83, 87
];

?

On would it rather be:

(B)

var primes = [
  2,
  3,
  5,
  ...
];

?

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent


I'd guess (A) based on the principle that that makes the fewest changes to the original code.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


Good deal. (And phew!) [A] it is.

I'll make it be!


Set owner to @pq.
Added Accepted label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/8616782?v=3" align="left" width="48" height="48"hspace="10"> Comment by clayberg


Removed this from the 1.2 milestone.
Added this to the 1.3 milestone.
Removed Type-Defect label.
Added Type-Enhancement label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/67586?v=3" align="left" width="48" height="48"hspace="10"> Comment by pq


Removed this from the 1.3 milestone.
Added this to the 1.4 milestone.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2156198?v=3" align="left" width="48" height="48"hspace="10"> Comment by kasperl


Removed this from the 1.4 milestone.
Added this to the 1.5 milestone.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2156198?v=3" align="left" width="48" height="48"hspace="10"> Comment by kasperl


Removed this from the 1.5 milestone.
Added this to the 1.6 milestone.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2156198?v=3" align="left" width="48" height="48"hspace="10"> Comment by kasperl


Removed this from the 1.6 milestone.
Added Oldschool-Milestone-1.6 label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2156198?v=3" align="left" width="48" height="48"hspace="10"> Comment by kasperl


Removed Oldschool-Milestone-1.6 label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent


Added AssumedStale label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants