Add ability to break for top member expression #1036

Merged
merged 1 commit into from Apr 11, 2017

Conversation

Projects
None yet
2 participants
@vjeux
Collaborator

vjeux commented Mar 18, 2017

It turns out that the top member expression doesn't go through the member chain logic. Let's give it the ability to break for now.

Fixes #1031
Fixes #1100

- doc.expandedStates.length - 1
-];
+const veryVeryVeryVeryVeryVeryVeryLong = doc.expandedStates
+ [doc.expandedStates.length - 1];

This comment has been minimized.

@jlongster

jlongster Mar 20, 2017

Member

This is very weird formatting

@jlongster

jlongster Mar 20, 2017

Member

This is very weird formatting

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

You are right, we shouldn't do that for computed properties.

@vjeux

vjeux Apr 6, 2017

Collaborator

You are right, we shouldn't do that for computed properties.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Mar 20, 2017

Member

I'm pretty sure this was intentional. foo.bar should not break. To me, it's seen as a normal identifier. It's only when you have many of them (foo.bar.baz.boo) that we want to break. Otherwise there are some pretty weird styles that we'll output by just moving around the bar in foo.bar.

Member

jlongster commented Mar 20, 2017

I'm pretty sure this was intentional. foo.bar should not break. To me, it's seen as a normal identifier. It's only when you have many of them (foo.bar.baz.boo) that we want to break. Otherwise there are some pretty weird styles that we'll output by just moving around the bar in foo.bar.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 6, 2017

Collaborator

There are three main cases when member expressions exist:

  • Inside of a CallExpression. We have some very specialized logic to handle this such that it only breaks when we want to. The logic is complicated but we haven't got much reports about it nowadays.
  • On-top of a CallExpression. Most of the time this is going to fit but in some cases this isn't and I can't see a reason why it shouldn't break.
  • With just a series of MemberExpressions. When that happens, this is likely seen as an identifier. If we are in a position where this needs to break, this means that all the parents did already break and if we don't, we'd go over 80 columns. This shouldn't change any existing behavior.

Given those, I think that it makes sense to allow breaking here. It's only going to prevent prettier from going over 80 columns when we have a chance not to.

Collaborator

vjeux commented Apr 6, 2017

There are three main cases when member expressions exist:

  • Inside of a CallExpression. We have some very specialized logic to handle this such that it only breaks when we want to. The logic is complicated but we haven't got much reports about it nowadays.
  • On-top of a CallExpression. Most of the time this is going to fit but in some cases this isn't and I can't see a reason why it shouldn't break.
  • With just a series of MemberExpressions. When that happens, this is likely seen as an identifier. If we are in a position where this needs to break, this means that all the parents did already break and if we don't, we'd go over 80 columns. This shouldn't change any existing behavior.

Given those, I think that it makes sense to allow breaking here. It's only going to prevent prettier from going over 80 columns when we have a chance not to.

@vjeux vjeux referenced this pull request Apr 6, 2017

Closed

array access then assignment #1141

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

I don't quite follow. :) Member expressions are just as common as variable names - they are everywhere! I think the getters_and_setters_enabled snapshot test is an example of where I think it's worse. If it's a single member expression like foo.bar I think it should always stay in tact, because in real code bar could be something longer and when it's nested deeply it hits the 80 (or whatever) limit. I think it looks weird when there's a single name on one line and we move the . down.

This does seem to fix other cases that should be breaking though. Like this one: SomeVeryLongUpperCaseConstant.someVeryLongCallExpression().some_very_long_member_expression. We could check for the case where a top-level member expression has an identifier for left & right nodes and not break there. Do you not think it's worth that though?

Member

jlongster commented Apr 11, 2017

I don't quite follow. :) Member expressions are just as common as variable names - they are everywhere! I think the getters_and_setters_enabled snapshot test is an example of where I think it's worse. If it's a single member expression like foo.bar I think it should always stay in tact, because in real code bar could be something longer and when it's nested deeply it hits the 80 (or whatever) limit. I think it looks weird when there's a single name on one line and we move the . down.

This does seem to fix other cases that should be breaking though. Like this one: SomeVeryLongUpperCaseConstant.someVeryLongCallExpression().some_very_long_member_expression. We could check for the case where a top-level member expression has an identifier for left & right nodes and not break there. Do you not think it's worth that though?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

For getters_and_setters_enabled, it used to go > 80 columns, now it doesn't anymore. It is indeed better to keep in a single line, but if we have an opportunity to break, I strongly believe that we should break and not go over 80 columns.

Collaborator

vjeux commented Apr 11, 2017

For getters_and_setters_enabled, it used to go > 80 columns, now it doesn't anymore. It is indeed better to keep in a single line, but if we have an opportunity to break, I strongly believe that we should break and not go over 80 columns.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

We should only break when it makes sense to; the max length is really at its core a suggestion. If we think the code looks worse when it breaks, I'd rather decide to let it overflow and let it look good rather than strictly enforce a line limit. Here I believe what will happen the most is that only 2-3 characters will go over the limit which doesn't make much of a difference, but breaking it is a big change. Early on I remember running into this more than once (breaking foo.bar in deeply nested code looked really bad).

I agree that there are several other cases that need to be fixed though, like the other tests. I just think if we break simple object lookups that's going to come up a lot.

Member

jlongster commented Apr 11, 2017

We should only break when it makes sense to; the max length is really at its core a suggestion. If we think the code looks worse when it breaks, I'd rather decide to let it overflow and let it look good rather than strictly enforce a line limit. Here I believe what will happen the most is that only 2-3 characters will go over the limit which doesn't make much of a difference, but breaking it is a big change. Early on I remember running into this more than once (breaking foo.bar in deeply nested code looked really bad).

I agree that there are several other cases that need to be fixed though, like the other tests. I just think if we break simple object lookups that's going to come up a lot.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

One thing we can do in addition to that is to allow a newline after = for member expressions like we now do for strings. This way it's first going to add a \n and then going to break if nothing else works.

Many people still enable the 80 columns lint rule with prettier and all the ones where prettier goes over 80 warn. It is annoying to people when you can't do anything about it like in this case as they have to add // prettier-ignore or // eslint-disable-next-line max-len.

Collaborator

vjeux commented Apr 11, 2017

One thing we can do in addition to that is to allow a newline after = for member expressions like we now do for strings. This way it's first going to add a \n and then going to break if nothing else works.

Many people still enable the 80 columns lint rule with prettier and all the ones where prettier goes over 80 warn. It is annoying to people when you can't do anything about it like in this case as they have to add // prettier-ignore or // eslint-disable-next-line max-len.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

I would love to get examples where it goes more than 80 columns and this is acceptable to you if you know how to find them back.

Collaborator

vjeux commented Apr 11, 2017

I would love to get examples where it goes more than 80 columns and this is acceptable to you if you know how to find them back.

vjeux added a commit to vjeux/prettier that referenced this pull request Apr 11, 2017

Allow to break for member expressions after =
This should address a concern of #1036
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

I believe that #1188 is going to address the concern that you have regarding this PR.

Collaborator

vjeux commented Apr 11, 2017

I believe that #1188 is going to address the concern that you have regarding this PR.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

The basic one is when you embed a long URL as a string that's a lot longer than 80 chars. People really should not be using the max-len eslint rule for prettier. You won't be able to do anything about it, but you shouldn't care! There's not much gain in being so strict about literally less than the max line length, and in fact hurts readability in some cases.

This is another case where I think it will hurt readability, but I can't give you a concrete example until I use it and hit it in real-world code. Other rare cases are when you are really indented and prettier just can't break up anything else (a long variable name just goes over). Ideally you wouldn't be that indented, but sometimes it happens and it's ok.

Again, I really think we should recommend turning of the max-len rule with prettier.

Member

jlongster commented Apr 11, 2017

The basic one is when you embed a long URL as a string that's a lot longer than 80 chars. People really should not be using the max-len eslint rule for prettier. You won't be able to do anything about it, but you shouldn't care! There's not much gain in being so strict about literally less than the max line length, and in fact hurts readability in some cases.

This is another case where I think it will hurt readability, but I can't give you a concrete example until I use it and hit it in real-world code. Other rare cases are when you are really indented and prettier just can't break up anything else (a long variable name just goes over). Ideally you wouldn't be that indented, but sometimes it happens and it's ok.

Again, I really think we should recommend turning of the max-len rule with prettier.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

@vjeux I don't see how that PR is specifically related? It's one case that might break, but I remember doing something like { foo: longObjectName.longProperty } in a deeply indented place and it moved .longProperty to the next line which I think looks really bad. It can break anywhere, not just after = I think those kinds of expressions need to be treated as if it were a single variable name, just like we don't try to break up longObjectNameLongProperty.

Member

jlongster commented Apr 11, 2017

@vjeux I don't see how that PR is specifically related? It's one case that might break, but I remember doing something like { foo: longObjectName.longProperty } in a deeply indented place and it moved .longProperty to the next line which I think looks really bad. It can break anywhere, not just after = I think those kinds of expressions need to be treated as if it were a single variable name, just like we don't try to break up longObjectNameLongProperty.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

Okay, let me change this PR to allow for breaking if it's after a call (which is what triggered my issue from the Facebook-side of things). I asked the person that posted about the member chain to give a more concrete use case that we can talk about (it's really hard without seeing some real code!).

I think that with #1188 and the version I'm suggesting, we should be in a good shape.

Collaborator

vjeux commented Apr 11, 2017

Okay, let me change this PR to allow for breaking if it's after a call (which is what triggered my issue from the Facebook-side of things). I asked the person that posted about the member chain to give a more concrete use case that we can talk about (it's really hard without seeing some real code!).

I think that with #1188 and the version I'm suggesting, we should be in a good shape.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

Okay done, it doesn't fix #1100 anymore.

Collaborator

vjeux commented Apr 11, 2017

Okay done, it doesn't fix #1100 anymore.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

I don't have much context here to know why it doesn't fix #1100, but to be clear I think we should allow breaking if there are multiple looks up like foo.bar.baz, or possibly even just allow lookups if it's anything other than foo.bar (foo.bar[0] could still break), meaning both the left and right-hand side are identifiers. Does that make sense?

Member

jlongster commented Apr 11, 2017

I don't have much context here to know why it doesn't fix #1100, but to be clear I think we should allow breaking if there are multiple looks up like foo.bar.baz, or possibly even just allow lookups if it's anything other than foo.bar (foo.bar[0] could still break), meaning both the left and right-hand side are identifiers. Does that make sense?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

I just changed it to only break if it's a call expression under. But if you think it should break if there's more than one chained, let me do that as well.

Collaborator

vjeux commented Apr 11, 2017

I just changed it to only break if it's a call expression under. But if you think it should break if there's more than one chained, let me do that as well.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

Yeah, I think I'm mainly annoyed with just this specific case:

longObject
  .longProperty

Whenever I've seen that it just seems like it went too far! Feel free to special case the above case though and allow anything else!

Member

jlongster commented Apr 11, 2017

Yeah, I think I'm mainly annoyed with just this specific case:

longObject
  .longProperty

Whenever I've seen that it just seems like it went too far! Feel free to special case the above case though and allow anything else!

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

Okay, updated. Not breaking anymore for just foo.bar, which also fixes #1100 again. I also think that we should do #1188.

Collaborator

vjeux commented Apr 11, 2017

Okay, updated. Not breaking anymore for just foo.bar, which also fixes #1100 again. I also think that we should do #1188.

Add ability to break for top member expression
It turns out that the top member expression doesn't go through the member chain logic. Let's give it the ability to break for now.

Fixes #1031
@@ -232,9 +232,22 @@ function genericPrintNoParens(path, options, print, args) {
path.call(print, "right")
]);
case "MemberExpression": {
+ const parent = path.getParentNode();
+ const shouldInline =
+ n.computed ||

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

Are computed member expressions always inlined no matter what?

@jlongster

jlongster Apr 11, 2017

Member

Are computed member expressions always inlined no matter what?

This comment has been minimized.

@vjeux

vjeux Apr 11, 2017

Collaborator

Yes, the [ is inline, but the content inside can break.

somethingLong[
  somethingLong
]

looks better than

somethingLong
  [somethingLong]
@vjeux

vjeux Apr 11, 2017

Collaborator

Yes, the [ is inline, but the content inside can break.

somethingLong[
  somethingLong
]

looks better than

somethingLong
  [somethingLong]

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

Ah right

@jlongster

jlongster Apr 11, 2017

Member

Ah right

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

Looks good to me now! Thanks!

Member

jlongster commented Apr 11, 2017

Looks good to me now! Thanks!

@vjeux vjeux merged commit 523f64f into prettier:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

vjeux added a commit that referenced this pull request Apr 11, 2017

Allow to break for member expressions after = (#1188)
This should address a concern of #1036

vjeux added a commit to vjeux/prettier that referenced this pull request Apr 13, 2017

vjeux added a commit to vjeux/prettier that referenced this pull request Apr 13, 2017

jlongster added a commit that referenced this pull request Apr 14, 2017

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