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

[[FIX]] Handle multi-line template literal return statements #2142

Closed
wants to merge 2 commits into from

Conversation

caitp
Copy link
Member

@caitp caitp commented Feb 1, 2015

Closes #1814

@caitp
Copy link
Member Author

caitp commented Feb 1, 2015

@jugglinmike could you review this one? it was the quickest hack i could come up with to support multi-line tokens in return statements

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 94.75% when pulling 67d6bef on caitp:issue-1814 into 33612f8 on jshint:master.

@jugglinmike
Copy link
Member

Hey Cait! It looks like this is indicative of a larger problem that exists for all multi-line structures:

jshint/src/lex.js

Lines 1377 to 1386 in 33612f8

// Methods that work with multi-line structures and move the
// character pointer.
var match = this.scanComments() ||
this.scanStringLiteral(checks) ||
this.scanTemplateLiteral(checks);
if (match) {
return match;
}

For instance, this also fails:

/* jshint multistr: true */
function foo() {
  return 'start\
    end';
};

I know your patch is intended as a quick hack, but what would you say to trying to resolve the problem a bit more generally? Here's what I'm thinking that would look like:

Multiline structures would need to track their last line and first line. I think it would make the most sense to change the current semantics around and have line refer to the first line. A new lastLine property (and maybe lastLineChar?) would be defined for multi-line structures only. (We could also try keeping line behavior as-is and defining a new firstLine property, but I think that would be more confusing.)

Do you think it would be that easy? Or maybe not and better to try in a follow up patch?

@caitp
Copy link
Member Author

caitp commented Feb 1, 2015

@jugglinmike in general, I don't think it is a problem. It is a problem for templates, because a single template token can span multiple lines. But as far as I'm aware, no other token fits that category

@jugglinmike
Copy link
Member

@caitp It's also a problem for multi-line string literals.

@caitp
Copy link
Member Author

caitp commented Feb 2, 2015

few extra bits added to work for string literals too... i think this can be generalized more cleanly, but i'll do that in a different patch

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 94.75% when pulling c794980 on caitp:issue-1814 into 33612f8 on jshint:master.

leebyron added a commit to leebyron/jshint that referenced this pull request Feb 5, 2015
This is a slight alteration of jshint#2142 to cleanly rebase atop the previous diffs adding context, allowing it to work with nested templates.

Fixes: jshint#1814
@caitp caitp closed this in 5c9c7fd Feb 6, 2015
jugglinmike pushed a commit to jugglinmike/jshint that referenced this pull request Feb 23, 2015
This is a slight alteration of jshint#2142 to cleanly rebase atop the previous diffs adding context, allowing it to work with nested templates.

Closes jshint#1814
Closes jshint#2142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants