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

To break or not to break (every possible line break emitted) #4091

Closed
cklein05 opened this issue Mar 2, 2018 · 3 comments
Closed

To break or not to break (every possible line break emitted) #4091

cklein05 opened this issue Mar 2, 2018 · 3 comments
Labels
lang:javascript Issues affecting JS 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

Comments

@cklein05
Copy link

cklein05 commented Mar 2, 2018

Prettier 1.11.0
Playground link

Input:

Bundler.prototype._mangleModuleFilename = function (relpath, suffix = '') {
    return path.dirname(relpath) + path.sep + this.filePrefix
        + path.basename(relpath) + suffix + this.fileSuffix
        + this.fileExtension;
};

Output:

Bundler.prototype._mangleModuleFilename = function(relpath, suffix = "") {
  return (
    path.dirname(relpath) +
    path.sep +
    this.filePrefix +
    path.basename(relpath) +
    suffix +
    this.fileSuffix +
    this.fileExtension
  );
};

Expected behavior:
Prettier seems to emit every possible line break, once a line does not fit into --print-width characters (there are exceptions). For my mind, this makes code less readable, since many logical lines then cover a lot of vertical space in my editor window.

Of, course, prettier is opinionated an one may argue, that the broken line itself is quite readable. However, considering all lines of the code, for me it would make more sense to add really necessary line breaks only, that is, to fill lines up to the specified print width. Actually, that is what is stated on the Options page:

For readability we recommend against using more than 80 characters:
[...]
Prettier, on the other hand, strives to fit the most code into every line. With the print width set to 120, prettier may produce overly compact, or otherwise undesirable code.

For the cases mentioned here, that's actually not true. Have a look at this other example:

Input:

Bundler.prototype._writeFile = function(filename, data, modulesWritten) {
    // Variables ommited.
    for (var i = 0; i < data.modules.length; i++) {
        var module = data.modules[i];
        if (module.path) {
            importStr += inlineModule === true ? newline : '';
            inlineModule = false;
            importStr += 'import * as ' + module.name + ' from \''  + curdirStr
                + this._makeRelative(filename, module.path) + '\';' + newline;
        } else if (module.expression) {
            importStr += inlineModule === false ? newline : '';
            inlineModule = true;
            importStr += 'const ' + module.name + ' = ' + module.expression
                + ';' + newline;
        }
        exportStr += first ? indent + module.name : ',' + newline + indent
            + module.name;
        first = false;
    }
};

Output:

Bundler.prototype._writeFile = function(filename, data, modulesWritten) {
  // Variables ommited.
  for (var i = 0; i < data.modules.length; i++) {
    var module = data.modules[i];
    if (module.path) {
      importStr += inlineModule === true ? newline : '';
      inlineModule = false;
      importStr +=
        'import * as ' +
        module.name +
        " from '" +
        curdirStr +
        this._makeRelative(filename, module.path) +
        "';" +
        newline;
    } else if (module.expression) {
      importStr += inlineModule === false ? newline : '';
      inlineModule = true;
      importStr +=                 // this is formatted differently
        'const ' + module.name + ' = ' + module.expression + ';' + newline;
    }
    exportStr += first
      ? indent + module.name
      : ',' + newline + indent + module.name;
    first = false;
  }
};

In this example, you can see that not every possible line break is emitted in all cases. The 3rd assignment to importStr is formatted differently, since the whole right hand expression fits into a single line. However, filling up lines consequently in all cases could produce more consistently formatted code.

I'd really appreciate using Prettier, since it seems to run quite stable and predictable (compared to other JavaScript formatters). Is there any chance to get this changed in the near future? Possibly, a --fill-lines-before-break (or similar) option could be of help for those cases.

@j-f1 j-f1 added status:needs discussion Issues needing discussion and a decision to be made before action can be taken lang:javascript Issues affecting JS labels Mar 2, 2018
@suchipi
Copy link
Member

suchipi commented Mar 2, 2018

Hello,

Prettier decides to add line breaks based on a complex variety of rules by transforming your input into a tree of "doc" structures, which are documented here: https://github.com/prettier/prettier/blob/03292a6488ce4da1dc19215f0632b0b764728900/commands.md

The doc command/node that exhibits the behavior you see is probably group: https://github.com/prettier/prettier/blob/03292a6488ce4da1dc19215f0632b0b764728900/commands.md#group

Changing such a far-reaching aspect of our printing algorithm via an option would be very nontrivial to implement and nearly impossible to maintain.

Issues around adjusting specific behavior, such as "when concatting a lot of strings, don't put a line break between each", are actionable/discussable, but this one is too vague for us to be able to do anything about.

@suchipi suchipi closed this as completed Mar 2, 2018
@cklein05
Copy link
Author

cklein05 commented Mar 3, 2018

Hello,

actually, that's not especially about concatting lots of strings, but seems a general behavior of Prettier to either don't break (if the whole line fits) or to break it at all possible locations (at least the tree's current siblings, child expressions are not broken if they fit). The string concatting code above is just an example. But that's also true for calling functions with lots of arguments (you actually show such an example on the introductional page) or for array literals, for example.

My idea is to generally fill up each line (as stated on the Options page for --print-width), even if the whole logical line does not fit, that is, breaking each oversized line into parts, each filling its line as much as possible.

foo(
  reallyLongArg(),
  omgSoManyParameters(),
  IShouldRefactorThis(),
  isThereSeriouslyAnotherOne()
);

Better:

foo(reallyLongArg(), omgSoManyParameters(), IShouldRefactorThis(),
  isThereSeriouslyAnotherOne());

That may be nontrivial, of course, but it's not vague. The general rule is to use each line's room as much as possible.

@duailibe
Copy link
Member

duailibe commented Mar 5, 2018

seems a general behavior of Prettier to either don't break (if the whole line fits) or to break it at all possible locations (at least the tree's current siblings, child expressions are not broken if they fit)

Yes that's a correct overview of how Prettier works and seems to match most of the code styles we see in the community. While I agree there are some cases where your proposed style is better (and we have infrastructure in place to format like that, like text inside JSX), it's probably not gonna happen because:

  • you want all your code to be formatted like this (unlikely, I guess) - we'd have to change lots of places to work with the fill algorithm and it's a complexity we don't want to include for such a rare code style
  • you want specific parts of your code formatted like this - ideally we'd have heuristics to automatically detect when we should print one way or another (really hard to come up with that); or add special handling of options through the code (like comments before AST nodes) and we won't add that for a number of reasons

Hope you can understand this decision. Thanks!

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:javascript Issues affecting JS 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
Projects
None yet
Development

No branches or pull requests

4 participants