Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Follow-up on Decorators PR #590

Merged
merged 5 commits into from
Jun 27, 2017
Merged

Follow-up on Decorators PR #590

merged 5 commits into from
Jun 27, 2017

Conversation

peey
Copy link
Contributor

@peey peey commented Jun 23, 2017

Q A
Bug fix? yes
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #524, #14
License MIT
  1. commit 1 Add a more helpful error message for when @dec export class {} is encountered, suggesting the user to use export @dec class {} instead (only happens in the new decorators2 plugin). As requested by @hzoo
  2. commit 2 Fixes Multiple Decorators with Class as Key #524 by using a stack of decorators to allow nesting instead of using one global list of decorators. Earlier the following code would have attached both @outer and @inner to Foo but now it works correctly
    @outer(@inner class Foo {})
    class Bar {}
  3. commit 3 adds the only useful test case I found in fixes #13: limit decorator use to CallExpression and IdentifierReference #14. Rest of tests are either already present or refer to an outdated version of the spec. It can now be closed.

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

👍

@@ -173,6 +173,11 @@ export default class StatementParser extends ExpressionParser {
return;
}

// special error for the common case of @dec export class
if (!allowExport && this.match(tt._export)) {
this.raise(this.state.start, "Using the export keyword between decorators and class is disallowed. Please use `export @dec class` instead");
Copy link
Member

Choose a reason for hiding this comment

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

Nit, might read a little better as:

Using the export keyword between a decorator and a class is not allowed. Please use export @dec class instead"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think that's better too. Will make changes and push it

@hzoo hzoo requested review from diervo and bakkot June 27, 2017 16:11
if (!allowExport && this.match(tt._export)) {
this.raise(this.state.start, "Using the export keyword between a decorator and a class is not allowed. Please use `export @dec class` instead");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd combine this with the above clause, personally -

if (this.match(tt._export)) {
  if (allowExport) {
    return;
  }
  this.raise(this.state.start, "Using the export keyword between a decorator and a class is not allowed. Please use `export @dec class` instead");
}

// Leading decorators.
decorators: Array<N.Decorator>;
// Leading decorators. Last element of the stack represents the decorators in current context.
// Supports nesting of decorators, e.g. @foo(@bar class {}) class {}
Copy link
Contributor

@bakkot bakkot Jun 27, 2017

Choose a reason for hiding this comment

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

I'd clarify here that foo applies only to the second class and bar only to the first.

@hzoo hzoo requested a review from littledan June 27, 2017 16:43
@bakkot
Copy link
Contributor

bakkot commented Jun 27, 2017

Haven't checked over all the output carefully, especially the source locations, but otherwise LGTM.

@peey
Copy link
Contributor Author

peey commented Jun 27, 2017

@hzoo do note that this isn't as big a change as the previous PR was so it wouldn't need as many reviews (in fact we could do with 2-3 reviews just like for any standard bugfix PR).

@hzoo
Copy link
Member

hzoo commented Jun 27, 2017

Yeah I don't think we need all the reviews just pinging people anyway 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple Decorators with Class as Key
4 participants