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

Don't force arrow function body on same line #760

Closed
Hilzu opened this issue Feb 21, 2017 · 8 comments · Fixed by #966
Closed

Don't force arrow function body on same line #760

Hilzu opened this issue Feb 21, 2017 · 8 comments · Fixed by #966
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:bug Issues identifying ugly output, or a defect in the program

Comments

@Hilzu
Copy link

Hilzu commented Feb 21, 2017

I created a function like this:

const promiseFromCallback = fn => 
  new Promise((resolve, reject) => 
    fn((err, result) => {
      if (err) return reject(err);
      return resolve(result);
    })
  );

This seems pretty clear to me with all the arrow function bodies moved to a new line. Prettier formats that function to this:

const promiseFromCallback = fn => new Promise((resolve, reject) => fn((
  err,
  result
) => {
  if (err) return reject(err);
  return resolve(result);
}));

At least to me that is a lot less readable than the first one. This formatting seems to be related to always moving the body of an arrow function to the same line as the arrow. I'm not sure this creates well formatted code.

I also added that code to prettier.github.io

@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2017

Thanks for the report! This is not intended to be printed like this. #674 and #680 are attempts at fixing the problem but we're not 100% sure what the root cause is.

@vjeux vjeux added the type:bug Issues identifying ugly output, or a defect in the program label Feb 21, 2017
@jlongster
Copy link
Member

Oh, those are related? I really need to look at those ;)

@vjeux
Copy link
Contributor

vjeux commented Feb 28, 2017

One thing I want to do at some point is to disallow last argument expansion if the params don't fit in a single line.

/* ... */ fn((
  err,
  result
) => {
  // ...
});

should be formatted as

/* ... */ fn(
  (err, result) => {
    // ...
  }
);

@vjeux
Copy link
Contributor

vjeux commented Mar 2, 2017

#847 makes it look:

const promiseFromCallback = fn =>
  new Promise((resolve, reject) => fn((err, result) => {
    if (err) return reject(err);
    return resolve(result);
  }));

instead. It's not the original one but looks way less broken than what is currently in prettier.

@Hilzu
Copy link
Author

Hilzu commented Mar 3, 2017

That new style looks much better. 👌🏻 The original function is formatted according to my personal preference but anything non-broken looking is good with prettier.

vjeux added a commit to vjeux/prettier that referenced this issue Mar 9, 2017
In practice, trying to allow calls to be inlined is causing a lot of code to look very weird and unstable as seen by the four issues this is fixing. It also requires us to add a conditional group and do hackery around it.

Fixes prettier#959
Fixes prettier#760
Fixes prettier#615
Fixes prettier#625
vjeux added a commit to vjeux/prettier that referenced this issue Mar 9, 2017
In practice, trying to allow calls to be inlined is causing a lot of code to look very weird and unstable as seen by the four issues this is fixing. It also requires us to add a conditional group and do hackery around it.

Fixes prettier#959
Fixes prettier#760
Fixes prettier#615
Fixes prettier#625
@vjeux vjeux closed this as completed in #966 Mar 9, 2017
vjeux added a commit that referenced this issue Mar 9, 2017
In practice, trying to allow calls to be inlined is causing a lot of code to look very weird and unstable as seen by the four issues this is fixing. It also requires us to add a conditional group and do hackery around it.

Fixes #959
Fixes #760
Fixes #615
Fixes #625
@jasonkuhrt
Copy link

@vjeux as @Hilzu had in his original, I too think it is more readable to to see function bodies nested more or less regardless of how small they are. For example I think this more readable:

const imageParams = src =>
  src.replace("__IMAGE_PARAMS__", "b_white")

than:

const imageParams = src => src.replace("__IMAGE_PARAMS__", "b_white")

I realize not everyone may agree so maybe this feature would be best as an opt-in flag. Is there an issue already tracking this described refinement or should I create a new one?

Thanks!

@spencerfdavis
Copy link

I'd like to add a +1 to @jasonkuhrt as well. I much prefer the body in a separate line. This option exist anywhere and maybe I'm just missing it?

@evilsoft
Copy link

evilsoft commented Sep 6, 2017

I would like to also second @jasonkuhrt and then on top of that third @spencerfdavis second of @jasonkuhrt comment.
(one step closer to elm 😉)

@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. type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants