Add noNewlineBetweenVar option

[AndyA]

Hi there,

Thanks for your excellent code!

I tend to have runs of var statements so it's useful for me to be able to suppress the blank lines between them. I've added a new option to do that: noNewlineBetweenVar (and a corresponding command switch "no-newline-between-var").

I hope it's OK.

Thanks!

[fidian]

This seems reasonable. In fact, it seems so reasonable that I think it should be the default behavior and we can exclude the option to turn it off. I love that you added it as an option and included tests! I'm also surprised that I didn't ignore the vim files earlier.

First a bit of discussion: What do you think about this being the default for all people and we just skip the option to disable it?

One thing your patch points out is that we lose the context of previous statements. Perhaps a better way to solve this problem in general is to track the context for all fragments, but I view that as adding unnecessary overhead when this condition would rarely be checked.

Typically when checking things like this I would inspect tokens, preferring to look backwards in the stream. Since the newlines are added at the point of the semicolon, perhaps it would be easier to look forward. One could modify tokenSemicolon() and change the needsTwoNewlines() function to just look ahead. It would be like something below.

var i;

if (oldContext === 'var') {
    i = index + 1;
    nextToken = tokenList[i];

    while (nextToken && (nextToken.type === 'WHITESPACE' || nextToken.type === 'LINE_TERMINATOR' || nextToken.type === 'MULTI_LINE_COMMENT' || nextToken.type === 'SINGLE_LINE_COMMENT') {
        i += 1;
        nextToken = tokenList[i];
    }

    if (nextToken && nextToken.type === 'KEYWORD' && nextToken.content === 'var') {
        return false;
    }

    return true;
}

This would require minor changes to tokenSemicolon() like so:

 * @param {prettyJs~Result} result
 * @param {complexionJs~ComplexionJsToken} token
 * @param {number} index
 * @param {Array.<complexionJs~ComplexionJsToken>} tokenList
 */
function tokenSemicolon(result, token, index, tokenList) {

Alternately, we could update tokenKeywordVar() to scan backwards, but we'd have to look past all sorts of crazy things to see if we were in a var declaration.

The last idea I had was to store the context on the semicolon tokens, but that initially seems like more work and more consumed memory and we'd only utilize this extra information in rare circumstances.

In the end I'm leaning strongly towards modifying tokenSemicolon() with code like above. Sadly, that would mean I would discard most of your work. What are your thoughts?

[AndyA]

On 18 Jun 2015, at 14:18, Tyler Akins [email protected] wrote:

This seems reasonable. In fact, it seems so reasonable that I think it should be the default behavior and we can exclude the option to turn it off. I love that you added it as an option and included tests! I'm also surprised that I didn't ignore the vim files earlier.

Yeah! I was shocked :)

First a bit of discussion: What do you think about this being the default for all people and we just skip the option to disable it?

Well, that would work for me - but my default setting was to avoid altering current behaviour.
One thing your patch points out is that we lose the context of previous statements. Perhaps a better way to solve this problem in general is to track the context for all fragments, but I view that as adding unnecessary overhead when this condition would rarely be checked.

Yeah, I guess that’s your call; does it make anything you have planned easier?
Typically when checking things like this I would inspect tokens, preferring to look backwards in the stream. Since the newlines are added at the point of the semicolon, perhaps it would be easier to look forward. One could modify tokenSemicolon() and change the needsTwoNewlines() function to just look ahead. It would be like something below.

var i;

if (oldContext === 'var') {
i = index + 1;
nextToken = tokenList[i];

while (nextToken && (nextToken.type === 'WHITESPACE' || nextToken.type === 'LINE_TERMINATOR' || nextToken.type === 'MULTI_LINE_COMMENT' || nextToken.type === 'SINGLE_LINE_COMMENT') {
    i += 1;
    nextToken = tokenList[i];
}

if (nextToken && nextToken.type === 'KEYWORD' && nextToken.content === 'var') {
    return false;
}

return true;

}
This would require minor changes to tokenSemicolon() like so:

• @param {prettyJs~Result} result
• @param {complexionJs~ComplexionJsToken} token
• @param {number} index
• @param {Array.<complexionJs~ComplexionJsToken>} tokenList
  */
  function tokenSemicolon(result, token, index, tokenList) {
  Alternately, we could update tokenKeywordVar() to scan backwards, but we'd have to look past all sorts of crazy things to see if we were in a var declaration.

The last idea I had was to store the context on the semicolon tokens, but that initially seems like more work and more consumed memory and we'd only utilize this extra information in rare circumstances.

In the end I'm leaning strongly towards modifying tokenSemicolon() with code like above. Sadly, that would mean I would discard most of your work. What are your thoughts?

I certainly don’t mind you discarding the work :)

Not sure if I have any useful input on the best way to proceed - your problem seems that there’s more than one way to do it - I’d just pick one. Given the quality of your test coverage any misfeatures would be easy enough to refactor in the future.

You got me thinking about code formatters in general though. I suspect if I was writing one from scratch I’d build a structure that’d be a cross between an AST and a DOM (which are not necessarily that different to start with). I’d want to be able to do things like balancing array literals (to make them square if they contain suitable number of elements) and horizontally aligning various vertical runs of punctuation - the ‘=‘ signs in a sequence of var foo = require(“…”) for example. But that’s not the question you asked :)

Andy Armstrong, Hexten