Fix regarding issue 2967

[drexpp]

This is a pull request for the issue #2967.

I wrote a simple test in test/anti-cases/invalid-character.pug

I am not sure if using else is better than adding an if statement with the error message since attributes starting with characterParser.isPunctuator(character) && !quoteRe.test(character) && character !== ':' (posted on the issue) is not same as doing else of the if expression.

I believe, there might be cases where right side this.assertExpression(...) might be true and left side false and will still print the new error message. (If it is the expected behaviour then it is okay).

if((!characterParser.isPunctuator(str[x]) || quoteRe.test(str[x]) || str[x] === ':') && this.assertExpression(val, true)){

[ForbesLindesay]

The test case looks good. I think this is the wrong location for the fix though. The code that tests for whether the attribute starts with a quote mark is

pug/packages/pug-lexer/index.js

Lines 1082 to 1086 in a01694b

	if(quoteRe.test(str[i])){
	quote = str[i];
	this.incrementColumn(1);
	i++;
	}

it should probably have an else if that errors if the attribute key starts with characterParser.isPunctuator(str[i]) && str[i] !== ':'. i.e. the first character of an attribute must always pass the test in

pug/packages/pug-lexer/index.js

Line 1221 in a01694b

if((!characterParser.isPunctuator(str[x]) || quoteRe.test(str[x]) || str[x] === ':') && this.assertExpression(val, true)){

(which could be a quote mark)

The code you are editing is parsing the attribute value, but you are looking to validate the attribute key.

[ForbesLindesay]

P.S. thanks for submitting the pull request, it makes it much clearer what's going on.

[ForbesLindesay]

Actually, since this is a breaking change, we should probably only make it a warning, rather than an error. We can then upgrade it to an error in the next major release.

To do this, the test case gets a little more complex. You can start with something a bit like https://github.com/pugjs/pug/blob/master/packages/pug/test/extends-not-top-level/index.test.js but you will need to do something like:

test('invalid attribute name should log warning', () => {
  const oldWarn = console.warn;
  const warnings = [];
  console.warn = warnings.push.bind(warnings);
  pug.compileFile(
    __dirname + '/index.pug'
  );
  console.warn = oldWarn;
  expect(warnings).toMatchSnapshot();
});

[drexpp]

Thanks for replying Forbes, I have a question, how to make a warning? (Tried to look around and only found pug-error). I am missing what kind of warning I should return from the modified statement like below.

if(quoteRe.test(str[i])){ 
   quote = str[i]; 
   this.incrementColumn(1); 
   i++; 
 }else if(characterParser.isPunctuator(str[i]) && str[i] !== ':'){

}

By now this is the test, basically what you did plus the import on top.

const pug = require('../../../');

test('invalid attribute name should log warning', () => {
  const oldWarn = console.warn;
  const warnings = [];
  console.warn = warnings.push.bind(warnings);
  pug.compileFile(
    __dirname + '/invalid-character.pug'
  );
  console.warn = oldWarn;
  expect(warnings).toMatchSnapshot();
});

Thanks as always!

[ForbesLindesay]

pug/packages/pug-parser/index.js

Lines 855 to 858 in 8248ba8

	console.warn(
	this.filename + ', line ' + tok.loc.start.line +
	':\nThe .jade extension is deprecated, use .pug for "' + node.file.path +'".'
	);

has an example of a warning. We generally just construct the string manually using the filename and line number and then call console.warn. We may want to add a better system for warnings at some point, but that's out of scope for this pull request.

[drexpp]

Oh okay I initially thought it was console.warn but thought it couldn't be like that since there was pug-error and was expecting pug-warn, anyway gonna try to make a new pull request with the changes you mentioned.

[drexpp]

Oops there must be something I might be doing wrong about the snapshot.

[drexpp]

@ForbesLindesay I don't know how to fix the push since there is a problem since the snapshot and the result of test produced by travis are not the same, I think it shouldn't take much time.

Anyway consider to close when the issue is fixed or the push is fixed and sent.

Thanks!

[ForbesLindesay]

This looks all good apart from that test

[ForbesLindesay]

You should also make sure you have the correct dependencies installed by running yarn && yarn bootstrap

[drexpp]

Hello Forbes this is really tricky, I added your fix (I tried in the line you mentioned and before the last line, but it made no difference), not sure what's wrong with file path and why when it is compared with the snapshot is not the same, because aparently it is the same path.

[rollingversions]

There is no change log for this pull request yet.

Create a changelog