disallowTrailingWhitespace: fix bugs by post processing

[lukeapage]

As noted in #1980 I think this is a better way of doing it.

All tests and fixes from #1978, #1979, #1980 are present and also add autofixing of the validate line breaks rule.

[markelog]

We already use two ways of autofixing - token-assert and _fix property, i would rather not introduce the third one.

Would you mind moving your autofixing logic to _fix, like that -

node-jscs/lib/rules/validate-quote-marks.js

Lines 136 to 141 in ec8c0b0

	errors.cast({
	message: 'Invalid quote mark found',
	line: token.loc.start.line,
	column: token.loc.start.column,
	additional: token
	});

and

node-jscs/lib/rules/validate-quote-marks.js

Lines 146 to 151 in ec8c0b0

	_fix: function(file, error) {
	var token = error.additional;
	var fixer = require(this._quoteMark === '"' ? 'to-double-quotes' : 'to-single-quotes');
	token.value = fixer(token.value);
	}

?

[markelog]

All tests and fixes from #1978, #1979, #1980

So we don't need those pulls? We can just land this one?

[lukeapage]

We already use two ways of autofixing - token-assert and _fix property, i would rather not introduce the third one.

Would you mind moving your autofixing logic to _fix, like that -

node-jscs/lib/rules/validate-quote-marks.js

Lines 136 to 141 in ec8c0b0

	errors.cast({
	message: 'Invalid quote mark found',
	line: token.loc.start.line,
	column: token.loc.start.column,
	additional: token
	});

and

node-jscs/lib/rules/validate-quote-marks.js

Lines 146 to 151 in ec8c0b0

	_fix: function(file, error) {
	var token = error.additional;
	var fixer = require(this._quoteMark === '"' ? 'to-double-quotes' : 'to-single-quotes');
	token.value = fixer(token.value);
	}

?

Looking at the _fix I don't see how that applies - _fix modifies tokens and the whole point of this is moving the autofixing out of tokens because modifying whitespace on these rules is alot more complicated with tokens (as you can see from the bugs I fixed in the other PR's).

if you don't want to accept a much simpler way of fixing these rules, we'll have to go back to fixing the buggy behaviour of the token fixer in #1978, #1979 and #1980 and abandon the fixing of line endings.

If I was going to fix line endings by modifying tokens I would first abstract the logic in js-file that finds the token to edit whitespace.. but then you would have "another way of autofixing" which you don't want - so that seems a bit pointless.

[lukeapage]

look at where _fix is done -

node-jscs/lib/string-checker.js

Line 266 in 3e13e68

this._fixJsFile(file, errors);

What you should be doing is joining together your two ways of autofixing, which look in most cases to be just simple refactorings of each other and allowing a 2nd way of autofixing which just changes the file after the fact.

There is a 3rd rule that will benefit from autoprocessing autofixing - disallowTabs. I don't think it really applies to anything else.

[markelog]

Postprocessing would be nice for it, but it kinda contradicts the idea of using AST/CST/tokens or parser itself. I'm not sure why we would need to go this way if we have such powerful tools already.

As of the modifying tokens or AST with _fix, it is not hard, for example, for the file like that -

var a;\n 

You will have following tokens array (with couple of our extension btw) -

[ Token {
    type: 'Keyword',
    value: 'var',
    start: 0,
    end: 3,
    loc: SourceLocation { start: [Object], end: [Object] },
    range: [ 0, 3 ] },
  { type: 'Whitespace', value: ' ', isWhitespace: true },
  Token {
    type: 'Identifier',
    value: 'a',
    start: 4,
    end: 5,
    loc: SourceLocation { start: [Object], end: [Object] },
    range: [ 4, 5 ] },
  Token {
    type: 'Punctuator',
    value: ';',
    start: 5,
    end: 6,
    loc: SourceLocation { start: [Object], end: [Object] },
    range: [ 5, 6 ] },
  { type: 'Whitespace', value: '\n \n', isWhitespace: true },
  { type: 'EOF',
    value: '',
    range: [ 9, 10 ],
    loc: { start: [Object], end: [Object] } } ]

So, you can just modify the Whitespace tokens, for which, we already have helper functions like setWhitespaceBefore, without introducing additional complexity to the system like post processing

[lukeapage]

but it kinda contradicts the idea of using AST/CST/tokens or parser itself.

It doesn't contradict it, it adds to it. Just because you have a complex way of doing something, why should you do it the complex way if there is a far simpler way of dong it?

Most of the style-checks in jscs make sense to use an AST, but these 3 rules don't.

I'm not sure why we would need to go this way if we have such powerful tools already.

Look at this PR. The AST way of doing it is 64 lines of code. Its difficult to understand and its very buggy. Even with my 3 other fixes I am not convinced it will always work.
The post processing is 10 lines of code. It will always work.

Its not about whats more powerful, its what makes sense for the job. File formatting like end of line and trailing whitespace does not make sense within the AST. If it did you would detect whitespace that way too - but you don't, you shortcut back to the source.

[lukeapage]

and this isn't coming from a dislike of AST or that way of doing it - I've made significant contributions to jshint and less.js, I know alot about parsing and processing and modifying AST's.

[hzoo]

I guess we just need to decide what we want to do while we move to eventually use the CST to check/fix in 3.0 #1854

[markelog]

Yeah, that is why i said that post processing is nice for such cases, but AST/CST/tokens exist to simplify any approach for modification or search of specific patterns, whitespaces are not that different from token Punctuator or VariableDeclarator AST node.

We don't use regexps so we wouldn't duplicate parser efforts and work not with source but with its representation of it. Which is much simpler approach in the long run.

Of course, i might be wrong, so in order to be sure. Let's ask other maintainers.

@hzoo, @mrjoelkemp, @mdevils thoughts?

[markelog]

I guess we just need to decide what we want to do while we move to eventually use the CST to check/fix in 3.0 #1854

Not really CST related question, post processing source output with regexps still could be a viable option with any representation of the code

[mdevils]

From my point of view, future of autofixing should be just an option autofix on the check method of a rule. Having CST, we can apply changes during checking (if necessary) and nothing will break. So I would avoid adding more complexity to the system and I'd like to keep it simple till we integrate CST.