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.

Sign up for GitHub

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

Jump to bottom

Make template token objects adhere token object structure #343

Merged
merged 3 commits into from
Jul 10, 2017
Merged

Make template token objects adhere token object structure #343

merged 3 commits into from
Jul 10, 2017

Conversation

iancmyers
Copy link
Contributor

While fixing an issue in eslint-plugin-react (jsx-eslint/eslint-plugin-react#1061), I noticed that template tokens differ from other tokens. A typical token looks like this:

Token {
  type: 'String',
  value: '"bar"',
  start: 44,
  end: 49,
  loc:
   SourceLocation {
     start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }

A template token looks like this:

{ type: 'Template',
  value: '`bar`',
  loc:
   { start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }

I've not be able to figure out why templates are plain JavaScript objects and not of the class type Token (aside from the fact that the template tokens are constructed differently in the token translator.

This fix copies the range values into start and end properties on the template tokens to make them adhere to the same structure as other token objects.

Fixes #319

@jsf-clabot
Copy link

jsf-clabot commented Jun 19, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

ljharb
ljharb suggested changes Jun 19, 2017
View reviewed changes
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance of a regression test? :-D

lib/token-translator.js Outdated
@@ -58,6 +58,8 @@ function convertTemplatePart(tokens, code) {

if (firstToken.range) {
token.range = [firstToken.range[0], lastTemplateToken.range[1]];
token.start = firstToken.range[0];
token.end = lastTemplateToken.range[1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nbd but maybe token.range = [token.start, token.end] after these lines would reduce some of the duplication?

@iancmyers
Copy link
Contributor Author

@ljharb I'd be happy to add a test, but I'd need to be pointed in the correct direction.

@ljharb
Copy link
Contributor

ljharb commented Jun 20, 2017

I think perhaps the tokenize fixtures might need start and end added alongside range and loc, but I'm not 100% confident :-/

@iancmyers
Copy link
Contributor Author

@ljharb Looks like the tester explicitly strips out start and end and the root level of the token object: https://github.com/eslint/espree/blob/master/tests/lib/tester.js#L28

iancmyers added 2 commits June 21, 2017 15:39
@iancmyers
Make template token objects adhere token object structure
fd4c2ac 
While fixing an issue in `eslint-plugin-react`
(jsx-eslint/eslint-plugin-react#1061),
I noticed that template tokens differ from other tokens. A typical token
looks like this:

```
Token {
  type: 'String',
  value: '"bar"',
  start: 44,
  end: 49,
  loc:
   SourceLocation {
     start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }
```

A template token looks like this:

```
{ type: 'Template',
  value: '`bar`',
  loc:
   { start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }
```

I've not be able to figure out why templates are plain JavaScript
objects and not of the class type `Token` (aside from the fact that the
template tokens are constructed differently in the token translator.

This fix copies the `range` values into `start` and `end` properties on
the template tokens to make them adhere to the same structure as other
token objects.

Fixes eslint#319
@iancmyers
Make template token objects adhere token object structure
289a848 
While fixing an issue in `eslint-plugin-react`
(jsx-eslint/eslint-plugin-react#1061),
I noticed that template tokens differ from other tokens. A typical token
looks like this:

```
Token {
  type: 'String',
  value: '"bar"',
  start: 44,
  end: 49,
  loc:
   SourceLocation {
     start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }
```

A template token looks like this:

```
{ type: 'Template',
  value: '`bar`',
  loc:
   { start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }
```

I've not be able to figure out why templates are plain JavaScript
objects and not of the class type `Token` (aside from the fact that the
template tokens are constructed differently in the token translator.

This fix copies the `range` values into `start` and `end` properties on
the template tokens to make them adhere to the same structure as other
token objects.

Fixes eslint#319
@iancmyers iancmyers force-pushed the icm-fix-token-start-end branch from cbe68e7 to 289a848 Compare June 21, 2017 22:42
@kaicataldo
Copy link
Member

If we're not testing these values anywhere else in our tests it seems to me like we shouldn't hold this change up for that. If we decide to merge this as is, I can make a follow up issue to see if we can make this testable somehow.

As @nzakas mentioned in the original response, this seems like a pretty clear bug and an easy, low risk fix.

@eslint/eslint-team Does anyone know why the tester does this?

@ljharb
Copy link
Contributor

ljharb commented Jun 29, 2017

@kaicataldo totally; my comment wasn't meant as a blocker, more killing time/seeing if the PR can be improved while waiting for a response from eslint core ;-) it'd be great to merge it straightaway!

@not-an-aardvark not-an-aardvark merged commit 3805aea into eslint:master Jul 10, 2017
@not-an-aardvark
Copy link
Member

Thanks for contributing!

not-an-aardvark pushed a commit that referenced this pull request Jul 10, 2017
@iancmyers @not-an-aardvark
Fix: Make template token objects adhere to token object structure (#343)
d4bdcb6 
While fixing an issue in `eslint-plugin-react`
(jsx-eslint/eslint-plugin-react#1061),
I noticed that template tokens differ from other tokens. A typical token
looks like this:

```
Token {
  type: 'String',
  value: '"bar"',
  start: 44,
  end: 49,
  loc:
   SourceLocation {
     start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }
```

A template token looks like this:

```
{ type: 'Template',
  value: '`bar`',
  loc:
   { start: Position { line: 4, column: 11 },
     end: Position { line: 4, column: 16 } },
  range: [ 44, 49 ] }
```

I've not be able to figure out why templates are plain JavaScript
objects and not of the class type `Token` (aside from the fact that the
template tokens are constructed differently in the token translator.

This fix copies the `range` values into `start` and `end` properties on
the template tokens to make them adhere to the same structure as other
token objects.

Fixes #319
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb requested changes

@kaicataldo kaicataldo kaicataldo approved these changes

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Template token objects differ from other token objects
5 participants
@iancmyers @jsf-clabot @ljharb @kaicataldo @not-an-aardvark