Consistency / Ease of refactoring / Commit lines discussion

[chrisblossom]

This is expanding on #74. Sorry if this is bike-shedding or if this has already been discussed in-depth. Feel free to close for any reason.

I think priority should be given to consistency (no surprises / reading and understanding code is "easier"), ease of refactoring, and reduced git-commit lines at the expense of what initially could be viewed as "looking" better.

Some of the changes below are probably too strict and / or out of scope.

Here are some examples / proposed changes.

1. Always expand objects / arrays.
   it is easier and more consistent to add/remove/update fields when objects are forced into always being on a new line. Similar to the reason to always include a trailing comma, it reduces commit messages making changes cleaner to review.

2. Do not omit parens with single input arrow functions.
   Added for consistency / ease of refactoring. It required when used with Flow / Typescript to add types and also hinders refactoring to add fields / change to deconstructing.

3. Ternary always on their own line

4. Deconstructed exports

5. If statements with && or || operators split into multiple lines

Exceptions (possible bike-shedding?):
The following would, I think, get in the way of reading and writing code.

1. import deconstructing
2. Function params
3. if statements without && / || operators

Given the following input (default prettier options + trailingComma):

import SomeModule, { test } from 'some-module';

const arr = ['printWidth', 'tabWidth'];

const obj = { first: 'one' };

const arrObj = [{ second: 'two' }, { third: 'three' }];

const otherObj = [
  { other: 1 },
  { o: 1 },
  { other: 1 },
];

const arrowFn = one => { return one };

function fn(one) {
  return one;
}

if (false) {
  console.log('never');
}

if(true && 'false' === 'false') {
   console.log('true');
}

const tern = obj ? '1' : '2';

export { otherObj };
export default arrowFn;

Would output:

import SomeModule, { test } from "some-module";

const arr = [
  "printWidth",
  "tabWidth",
];

const obj = {
  first: "one",
};

const arrObj = [
  {
    second: "two",
  },
  {
    third: "three",
  },
];

const otherObj = [
  {
    other: 1,
  },
  {
    o: 1,
  },
  {
    other: 1,
  },
];

const arrowFn = (one) => {
  return one;
};

function fn(one) {
  return one;
}

if (false) {
  console.log("never");
}

if (
  true &&
  "false" === "false"
) {
  console.log("true");
}

const tern = obj
  ? "1"
  : "2";

export {
  otherObj,
};
export default arrowFn;

[vjeux]

Thanks for raising this here. We absolutely want to have this discussion :)

My personal goal with prettier is to make it look like code that would have been written by a human programmer and to the extent possible, minimize changes to code that is already written. The objective is to make the transition easier from a non-prettified codebase.

To me, the changes that you are proposing are in theory good changes but I am very worried that it's going to add so many new lines everywhere that people are going to react negatively to it when it is actually applied to their own code.

[chrisblossom]

Thanks for taking the time to respond.

I definitely agree with your concern. That is what I was trying to put into consideration when writing "exceptions". Keep in mind this isn't meant to be an all-or-nothing topic or change. Just wanting to throw some ideas out there.

I started with just the object and arrow example, but (like everything else), the feature / topic creeped up.

Personally I have found myself wasting more time making objects/arrays fit around the data, which is why I think having them always expanded is preferred in the long run.

I had the same issue with the arrow parens. Especially when converting a project to Flow.

Should be noted I use AirBnb's eslint config (now slightly modified) with my personal projects.

[jlongster]

There are too many cases where collapsed objects/array are much nicer, and I don't buy the argument that's easier to add elements if they are on their own line. It's easy to go into the same line, add a comma, and add a new element. And with prettier, you never have to care if it gets too long; if it does it will automatically be broken up.

I want to avoid only applying rules to exceptional cases. We want you to write code and generally know how it's going to be formatted -- the syntactical context doesn't matter. Why expand exports, but not imports? It gets way too subjective, and it's better to be fully consistent.

Single-arg arrow functions are also extremely common and I don't think it's worth adding parens just to make it easier to add args later. It's really nice to just have arr.map(x => ...).

Thanks for raising these issues, it is good to talk about, but I think we are settled on the most of the big questions of style. I think from here on we are only going to make tiny tweaks to some of the rare edge cases.

[smacker]

Is it possible to "remember" which style was used and use the same when do formatting? (unless it gets too long of course).

One example from my project, but I noticed many issues like this:

Input:

{ _renderField(allowTrackingNumberUpdate, 'Tracking number update', {
    componentClass: Radio,
    options: [
        { value: 'true', label: 'Allow' },
        { value: 'false', label: 'Disallow' },
    ],
}) }

Output:

{_renderField(allowTrackingNumberUpdate, 'Tracking number update', {
    componentClass: Radio,
    options: [{ value: 'true', label: 'Allow' }, { value: 'false', label: 'Disallow' }]
})}

In this case I would love to keep each option on new line.

[vjeux]

@smacker Right now we do "remember" the style for objects, where if it was expanded, it stays expanded. In your example, you would also want the same behavior to exist for arrays. We should discuss it too.

However, right now there's a bug with this logic where many cases are wrong. We are working on fixing it, you can see the technical discussion about it in #559

[jlongster]

Is it possible to "remember" which style was used and use the same when do formatting? (unless it gets too long of course).

Fwiw, the whole point of this project is to not remember the original formatting. There are cases where we've conceded, but in general we throw away all original formatting. That's a feature, not a bug. If it were up to just me, I wouldn't respect any of the original formatting, but there are too many diverse codebases and styles to do that, so we do things like keeping blank lines and object expansions. But in general we are going to ignore it because we want to enforce a consistent style.

[smacker]

@jlongster sometimes machine can't produce format that is easy to read for a human. Especially when we are talking about new lines, that people use to separate different blocks of logic. Tools should help, not prevent to write readable code.

Good example - gofmt. It doesn't throw away all original formatting, but every time when I read go code - it looks consistent enough. Even if that are very different projects from very different people.

[rafayepes]

Having objects/arrays in multiple lines makes it easier to code review too

[chrisblossom]

I think priority should be given to consistency (no surprises / reading and understanding code is "easier"), ease of refactoring, and reduced git-commit lines at the expense of what initially could be viewed as "looking" better.

@jlongster You used the words "much nicer" a couple times. This is exactly what I was trying to address with this issue. I think stylistic choices should decided on what is best for long-term code quality / maintenance instead of what is easier to initially write / nicer to initially look at.

There are too many cases where collapsed objects/array are much nicer, and I don't buy the argument that's easier to add elements if they are on their own line. It's easy to go into the same line, add a comma, and add a new element. And with prettier, you never have to care if it gets too long; if it does it will automatically be broken up.

You are right, adding an element is just as easy. What is more difficult is:

1. Commenting out / removing a single element

• It is easier to select entire code blocks over line positions
• For shortcuts, either you have to learn yet another shortcut to do an inline-comment (/* */), or you have to modify the element to be on a separate line.

2. Reviewing git changes

• Going from single line to multiline object will now consist of a much larger git diff than would be if it was just the element that was changed/added/removed.
• See exactly what changed. Less mental filtering.

3. Code review

• Consistency is easier to understand / look over
• Less mental filtering

I think that is a lot to give up when the only reason to keep it one line is so it initially looks better, which is subjective / personal preference / whatever you are used to.

I want to avoid only applying rules to exceptional cases. We want you to write code and generally know how it's going to be formatted -- the syntactical context doesn't matter. Why expand exports, but not imports? It gets way too subjective, and it's better to be fully consistent.

I agree. The majority of exceptional cases should be avoided.

For why expanded exports and not imports: I think it is much more likely for an export to be added/removed/changed/commented than an import. I think import lines are typically static, and aren't exactly considering the "meat" of the javascript file. But this was an initial response and possible "bike-shedding" / personal preference / experience.

Single-arg arrow functions are also extremely common and I don't think it's worth adding parens just to make it easier to add args later. It's really nice to just have arr.map(x => ...).

It is marginally "nicer" to look at, which is really just personal preference. It is confusing for people that are unfamiliar with arrow functions.

It is not consistent / easier to change later. Every other way to write that function requires parens. If parens were added, you can still write the function without parens. Less keys to start, and have prettier add them when formatting. Win-win

To recap, I think stylistic choices should be made to promote consistency, refactoring, and code sharing/reviewing over what could be considered as looking nicer.

[lydell]

For reference, here is a diff example (although for objects, not for arrays): http://eslint.org/docs/rules/object-property-newline

[Undistraction]

For me, not being allowed to choose expanded arrays and objects (and take advantage of the cleaner commits that result) is a deal-breaker. I also think that arguing that it 'looks better' is entirely subjective. I would much prefer to scan down a list of items than across one.

[newtriks]

I appreciate the work put in to this lib thank you. I understand this tiny tweaks are now the focus point for this library, but I have to comment and support the suggestion of object properties being on new lines due to the reasons made above. Readability really clearly suffers as soon as Objects reach a very small number of properties in my experience. I find new lines for each property important for code reviews, commits and general consideration for other devs coming into the codebase to quickly read and understand data structures they'll be dealing with.

[vjeux]

@Undistraction and @newtriks: if you put a \n anywhere in your object, it will be forced expanded. This way you have control over how to display it.

[Undistraction]

@vjeux For objects yes, but not for Arrays.

[jlongster]

I think I only said "much nicer" once :)

To recap, I think stylistic choices should be made to promote consistency, refactoring, and code sharing/reviewing over what could be considered as looking nicer.

Those are all just as subjective about what looks nicer. What looks nicer is what is more consistent and easier to share & review. I'm not after "nice" looking code at all; I built this project to enforce total consistency and make it way easier to share & review code.

If you have expanded an object, we will keep that object expanded now. Objects are used for a variety of thing in JS so we made an exception there. But for arrays we are still going to collapse it; if you want to talk about maintaining array expansion please open a new issue.

The rest of the things are small nits that I don't think we are going to change. If you want to talk more about any specific points, please open issues for each one individually.

[Undistraction]

Added a new issue for allowing expansion of Arrays by default: #817