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

Support mixins + includes statements #105

Merged
merged 6 commits into from
Nov 3, 2017
Merged

Support mixins + includes statements #105

merged 6 commits into from
Nov 3, 2017

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Nov 2, 2017

(This includes #103)

Fixes #92.

MixinMember only includes stringifiers but not static members, while current operation()/attribute() always include both. This PR pulls static_member() and stringifier() out of operation()/attribute() to support mixins + better match the parser structure to the spec.

MixinMember ::
    Const
    RegularOperation
    Stringifier
    ReadOnly AttributeRest

Bonus: This renames nonspecial_operation to regular_operation to match the spec naming.

@marcoscaceres marcoscaceres self-requested a review November 2, 2017 08:35
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

couple of nits, but this looks great!

"static": false,
stringifier: false
};
const ret = Object.assign({}, EMPTY_OPERATION);
Copy link
Member

Choose a reason for hiding this comment

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

this is nice :)

lib/webidl2.js Outdated
function stringifier(store) {
all_ws(store, "pea");
if (!consume(ID, "stringifier")) return;
const prefix = "stringifier";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just inline this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

lib/webidl2.js Outdated
@@ -863,7 +910,7 @@
}
};

const enum_ = function(store) {
const enum_ = function (store) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: whitespace after function

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, the VSCode formatter 😬

Copy link
Member

Choose a reason for hiding this comment

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

We should add .vstudio file at the base... I'm also using code, and we should start formatting this stuff properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, it should be function enum_ instead of const enum_. Omitted on the previous PR.

@marcoscaceres
Copy link
Member

@saschanaz, if you are happy to merge this, I'm happy to ship it and add support in ReSpec :) Let me know.

README.md Outdated

```JS
{
"type": "interface-mixin",
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering, maybe we should change this to "interface mixin"

Copy link
Member

Choose a reason for hiding this comment

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

(that would match "callback interface", and would allow me to use the type directly in ReSpec without needing to special case the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to keep consistency with line-comment, multiline-comment, etc., although they are undocumented.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Do we want to change them as line comment, multiline comment, etc.?)

Copy link
Member

Choose a reason for hiding this comment

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

Nah, line-comment and multiline-comment are ok.

Copy link
Member Author

@saschanaz saschanaz Nov 3, 2017

Choose a reason for hiding this comment

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

Changes made! (But why not line comment?)

Copy link
Member

Choose a reason for hiding this comment

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

Only because that would be a breaking change (e.g., in ReSpec, I have code that already expects "line-comment").

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

requesting change on type identifier.

@marcoscaceres
Copy link
Member

Happy to also make this change, let me know @saschanaz.

@saschanaz
Copy link
Member Author

saschanaz commented Nov 3, 2017

I don't see any problems now so it should be safe to merge 😁 (If you don't see other problems either)

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

🎊

@marcoscaceres marcoscaceres merged commit 6f86663 into develop Nov 3, 2017
@marcoscaceres marcoscaceres deleted the mixin branch November 3, 2017 02:18
@marcoscaceres
Copy link
Member

  • webidl2@8.0.0

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

Successfully merging this pull request may close these issues.

None yet

2 participants