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

Discussion: option to disable collapsing in more places #3524

Closed
rattrayalex opened this issue Dec 19, 2017 · 20 comments
Closed

Discussion: option to disable collapsing in more places #3524

rattrayalex opened this issue Dec 19, 2017 · 20 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Milestone

Comments

@rattrayalex
Copy link
Collaborator

From #3503 (comment), @ljharb (maintainer of airbnb-config-eslint) claimed this desire:

there's a number of places that multiline should be forced (the way prettier currently works), but in the cases prettier currently forces single-line, we want both options to be permitted, so the developer's explicit choice of single- or multi- line can be preserved. In other words, we want prettier to never collapse anything, only expand when needed.

When asked which cases, he claimed:

Every case: objects, arrays, jsx elements - anything that is explicitly chosen by the developer to be multiline, needs to remain multiline.

Note that prettier already preserves unnecessary multiline expands for objects; we could certainly do the same for arrays. Our JSX formatting is fairly flexible (eg; preserves extra newlines) but does collapse - it'd be fairly reasonable to disable collapsing there as well.

My first instinct is that this is not a good idea (particularly for chains, binary expressions, etc). But in a few limited cases, like arrays, it may be worth considering.

In any case, I wanted a forum for discussion separate from #3503.

@rattrayalex
Copy link
Collaborator Author

One example of something @ljharb probably did not mean when he said "everything:

if (
  true
) {
  return false;
}

@rattrayalex
Copy link
Collaborator Author

I'm curious if we've had many asks for this from users, and if so, what language features it's concentrated around.

@azz azz added this to the Airbnb Compat milestone Dec 19, 2017
@azz azz added status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. labels Dec 19, 2017
@rattrayalex
Copy link
Collaborator Author

rattrayalex commented Dec 19, 2017

Note that prettier's jsx handling does indeed preserve linebreaks sometimes but not others. (though JSX is a special snowflake in many ways; it encodes newlines directly in the AST, and has strange rules about when they matter).

@azz
Copy link
Member

azz commented Dec 19, 2017

We've actually had people ask for the opposite (ignore the original input entirely). And I think I recall a couple of requests for preserving multiline. But usually when they're using a print width too high.

@SimenB
Copy link
Contributor

SimenB commented Dec 19, 2017

If this is added, more than ever I want #2068. The whole point (for me) with prettier is that it creates a uniform code base. Adding all these exceptions because of current code style makes prettier less attractive to me.

@rattrayalex
Copy link
Collaborator Author

Yeah. My hunch is that this is one of those aspects of prettier you learn to love once you've used it for a while.

@ljharb
Copy link

ljharb commented Dec 19, 2017

@rattrayalex actually I did include #3524 (comment). Every single time a developer chooses multiline when single-line would suffice, that is an intentional choice I want to preserve. More importantly, it's not one that a later eslint pass can restore, since collapsing it destroys the information.

@duailibe
Copy link
Member

I don’t like very much what we do for object expressions now but I think that was a fair compromise. If we do this, it’d have to be behind an option and that, of all options we have to say no, would become a maintenence nightmare.

Instead, we should find out which cases developers wish to be expanded and try to print that better. Because more than maintenence burden, I think this goes against what I believe Prettier is about: developers giving up control of their formatting completely and never having to think about it anymore.

@ljharb
Copy link

ljharb commented Dec 19, 2017

@duailibe the cases are not syntactically determinable; one thing that choosing multiline connotes is the expectation of change, since multiline constructs with trailing commas have less diff churn when changes are made.

@bakkot
Copy link
Collaborator

bakkot commented Dec 19, 2017

@ljharb, all formatting decisions are intentional choices, but the whole point of this project is that you usually don't actually want to preserve them (especially when working on teams). Why is this particular kind of formatting decision different from any of the others prettier happily stomps?

@duailibe
Copy link
Member

duailibe commented Dec 19, 2017

@ljharb Understood.. for "expectation of changes", a developer can always express this with code (instead of whitespace):

Prettier 1.9.2
Playground link

Input:

if (
  fooBarBaz &&
  (fooBarBaz || fooBarBaz)
) {
}

if (
  // this may change!
  fooBarBaz &&
  (fooBarBaz || fooBarBaz)
) {
}

Output:

if (fooBarBaz && (fooBarBaz || fooBarBaz)) {
}

if (
  // this may change!
  fooBarBaz &&
  (fooBarBaz || fooBarBaz)
) {
}

@ljharb
Copy link

ljharb commented Dec 19, 2017

@bakkot because this category in particular creates excessive diff churn when adding an item or removing an item passes an arbitrary threshold.

@rattrayalex
Copy link
Collaborator Author

@ljharb you are aware of // prettier-ignore comments, I assume, correct? I do use them in rare cases, such as when I want a single very long array to be printed on one line or as an array.

@ljharb I think the problem that you'd find with this is that many newlines will be artifacts from prettier itself, not from intentionality at all. @lydell provided a decent example of this in #3503 (comment).

In practice, I find that it is very common for prettier to break up an expression into multiple lines, I take that as a hint to clean up my code, and I breathe a sigh of relief and gratitude when my function call or if statement snaps back into place.

@ljharb if you haven't tried writing code with prettier's "format-on-save" editor integration for at least a few days, you may consider doing so. I think you will find a few surprises.

(I am speaking as someone who never actually used prettier even after months as a maintainer, and while I always thought it was cool, couldn't stop saying "wow" once I started using it at work. It's a different effect than one might expect, in ways that are very pertinent to this issue).

@rattrayalex
Copy link
Collaborator Author

Ah, I see the trick of using "a comment at all" was mentioned, so my mention of // prettier-ignore was a bit redundant.

When something needs to be multiline, it really usually is as simple as doing this:

const arr = [
  // explanation
  1,
  2,
];

I don't find many times when I wish to cause a node to be multiline, but this typically works when I do; and there's a fairly high correlation between "I want this to be multiline" and "this needs a comment".

@jlongster
Copy link
Member

jlongster commented Dec 20, 2017

I still fully believe that this is contrary to the original design of prettier and it's better off as a fork. This is a monumental change. I'm not going to argue it's a bad way of doing things, or that there are positives you could argue for it, but I think it would be a net loss (obviously I'm biased).

For the current users of prettier I think we should make it clear that the majority of prettier's formatting is frozen. If you use it now it will continue to work that way for the future unless there's some extremely critical reason why it doesn't work.

I know I'd have to fork prettier to return it to the original way of working if this was changed.

@jlongster
Copy link
Member

(And no, I don't think an option is feasible. It dilates prettier's identity and would be a huge maintenance burden. Testing would be exponentially harder, bug reports much harder to figure out which mode was used, etc)

@jlongster
Copy link
Member

jlongster commented Dec 20, 2017

Also see #3503 (comment)

I don't think this makes any sense when combined with auto-expanding. You either want to respect the original developer's choice completely (no expanding or collapsing) or auto-expand and collapse. You don't want half of each.

@rattrayalex
Copy link
Collaborator Author

rattrayalex commented Dec 20, 2017 via email

@jlongster
Copy link
Member

@rattrayalex I actually would use the hell out it, but I'm not sure about adding it as an option. Personally I'd like to keep this as zero config as we can get and minimize the amount of differences. While it's not as big of a change as the this issue, it's still a lot bigger change than just single/double quotes, bracket spacing, etc. I don't feel as strongly about it though and if other maintainers decide it should be an option, you can do it. I'd vote against but wouldn't block that decision.

@rattrayalex
Copy link
Collaborator Author

Per #3524 (comment) I'll close this issue.

@ljharb if you find that adding comments doesn't work for your needs, and this truly is a critical feature for you, I would absolutely suggest a fork.

Let's continue the conversation about always collapsing objects (eg; the inverse of this issue) in #2068.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

No branches or pull requests

7 participants