tests-always-included / pretty-js

Beautify and pretty print JavaScript and JSON
Other
21 stars 4 forks source link

Functions assigned to variables are indented incorrectly #9

Open jacekradko opened 9 years ago

jacekradko commented 9 years ago

I would expect the following code to be indented as such:

var someFunction = function () {
    var a;
};

However, the actual output is

var someFunction = function () {
        var a;
    };

Here is a test scenario that fails:

{
  "input": "var someFunction = function (){var a;}",
  "output": "var someFunction = function () {\n    var a;\n}"
}

The output is correct if the variable is initialized before its value is set, like here:

var someFunction;
someFunction = function () {
    var a;
};

Here is a passing test scenario for this:

{
  "input": "var someFunction; someFunction = function (){var a;};",
  "output": "var someFunction;\n\nsomeFunction = function () {\n    var a;\n};"
}
fidian commented 9 years ago

This indentation is based on passing jslint's rules. You're still in context of the var. How would you like to indent these two bits of code?

var someFunction1 = function () { var  a; }, someFunction2 = function () { var a;};

and

var someFunction = function () { var a; }, anotherVariable;

Removing bug tag because this indentation is intentional. See pretty-js.spec.js line 79.

jacekradko commented 9 years ago

This is a separate case though. If I am only declaring a single variable, I don't expect it to get double indented just because there is a VAR in front of it. Otherwise it would always have to be like this:

var a = {
        something: true
    };      
fidian commented 9 years ago

That is exactly what it does right now.

screenshot from 2015-01-12 16 35 09

So, you're suggesting that a level of indentation is dropped if there is only one variable and preserve the indentation if there are multiple variables? I'd like to steer away from one-off rules as much as possible.

fidian commented 9 years ago

Oh, I should have mentioned that the first line of code was what I typed. Sorry for neglecting that.

fidian commented 9 years ago

Relevant comments from IRC:

(04:42:18 PM) deadlypole: http://jsbeautifier.org/ does what you mentioned, removes indentation if there is only a single variable being declared, and adds it for more.

(04:49:29 PM) fidian: My beautifier basically scans through the tokens without looking forwards or backwards, which makes this sort of thing difficult. I'd have to add special code at semicolons to see if I am closing a "var" context, then look back somehow to see if there was a comma. If there was no comma, reindent all lines after the "var" line and through the current line with one less indent.

(04:50:32 PM) fidian: The biggest challenge is that I need to find where the var context starts. I track what token started a context so I can end it, but I don't think I track where the token ends up.

(04:50:44 PM) deadlypole: That's fair. I figured it would be difficult.

(04:51:38 PM) deadlypole: Honestly, I am fine with leaving it as is. I can just put that in our styleguide to define all of you variables at the top of their respected contexts.

(04:53:08 PM) fidian: I do know that using jslint and it's "one var" type of rule has helped me notice when I was reusing variables or when I had too many variables for a concise and testable function. However, I don't think it is a proper response for the guy making a pretty printer to say that you should change your coding style. The pretty printer should work with anything and give you what you want (or try extremely hard).

(04:54:26 PM) fidian: I'll ponder this while I implement the other two.

The "other two" are #10 and #11.