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

Warn on missing attribute value end quotes #71

Merged
merged 7 commits into from Oct 12, 2017

Conversation

Projects
None yet
4 participants
@cherifGsoul
Copy link
Member

cherifGsoul commented Oct 5, 2017

this fixes #7

@phillipskevin
Copy link
Contributor

phillipskevin left a comment

Looks good @cherifGsoul. Just a couple small things.

@@ -695,3 +695,44 @@ test('camelCase properties are encoded with on:, :to, :from, :bind bindings', fu

parser("<h1 on:aB='c' dE:to='f' gH:from='i' jK:bind='l'></h1>", makeChecks(tests));
});

//!steal-remove-start
test('Warn on missing attribute value end quotes (canjs/can-view-parser#7)', function () {

This comment has been minimized.

Copy link
@phillipskevin

phillipskevin Oct 6, 2017

Contributor

Can you update this to use can-test-helpers and the testHelpers.dev.devOnlyTest function instead of steal-removeing the test. You can take a look at https://github.com/canjs/can-stache/blob/675fa5c3e9f09023b00566657bfce973654ec89c/test/stache-test.js#L200-L215 for an example.

This comment has been minimized.

Copy link
@cherifGsoul

cherifGsoul Oct 6, 2017

Author Member

Done!

@@ -271,12 +271,22 @@ var callAttrStart = function(state, curIndex, handler, rest){

var callAttrEnd = function(state, curIndex, handler, rest){
if(state.valueStart !== undefined && state.valueStart < curIndex) {
handler.attrValue(rest.substring(state.valueStart, curIndex));
var quotedVal = rest.substring(state.valueStart - 1, curIndex + 2);

This comment has been minimized.

Copy link
@phillipskevin

phillipskevin Oct 6, 2017

Contributor

can you move the quotedVal into the steal-remove-start/steal-remove-end?

This comment has been minimized.

Copy link
@cherifGsoul

cherifGsoul Oct 6, 2017

Author Member

Done!

canDev.warn = _warn;
};

makeWarnChecks('<my-input {value}="name" (value)="updateNameOnEven(%viewModel.value)/>', [

This comment has been minimized.

Copy link
@phillipskevin

phillipskevin Oct 9, 2017

Contributor

Sorry I wasn't clear. Can you use testHelpers.dev.willWarn also?

This comment has been minimized.

Copy link
@cherifGsoul

cherifGsoul Oct 9, 2017

Author Member

No problem, I'll fix it :)

@phillipskevin phillipskevin requested a review from christopherjbaker Oct 10, 2017

var quotedVal = rest.substring(state.valueStart - 1, curIndex + 2);
quotedVal = quotedVal.trim();
if (state.inQuote !== quotedVal.charAt(quotedVal.length - 1)) {
dev.warn("End quote is missing for " + val);

This comment has been minimized.

Copy link
@christopherjbaker

christopherjbaker Oct 10, 2017

Contributor

Should we add the filename and line number, like the other errors?

This comment has been minimized.

Copy link
@cherifGsoul

cherifGsoul Oct 10, 2017

Author Member

Filename and line number of the file caused the warning?

This comment has been minimized.

Copy link
@christopherjbaker

christopherjbaker Oct 10, 2017

Contributor

Sorry. Kevin asked me to review this, so I assumed it was his code. We've been trying to make debugging easier, and among that is to make errors more explanatory. As of today, these errors can have the filename and line number (though the filename is not always present).

This comment has been minimized.

Copy link
@cherifGsoul

cherifGsoul Oct 10, 2017

Author Member

No problem, for the line I take lineNo argument as it is or it should be processed?

This comment has been minimized.

Copy link
@christopherjbaker

christopherjbaker Oct 10, 2017

Contributor

Shouldn't have to do anything with it. Should just be able to add it on like in the example.

@imaustink imaustink self-requested a review Oct 12, 2017

@imaustink imaustink merged commit 784d691 into master Oct 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@imaustink imaustink deleted the warning_about_missing_end_quotes branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.