Skip to content

Conversation

@mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Sep 22, 2017

Fixes microsoft/vscode#34857

See microsoft/vscode#34857 for more details about this issue

@mjbvz mjbvz self-assigned this Sep 22, 2017
@mjbvz mjbvz requested a review from alexdima September 22, 2017 23:43
src/main.ts Outdated
String = 2,
RegEx = 4
RegEx = 4,
MetaEmbedded = 8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No encoded tokens will ever be marked as MetaEmbeded. It's just used internally to reset the scope. Let me know if you can think of a better way to express this

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is problematic because this enum makes it in the public module API

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 12, 2017

Hey @alexandrudima, can you please take a look at this PR and let me know if you have any feedback or alternate design suggestions

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 25, 2017

Ping @alexandrudima. I'd really like to see if we can address microsoft/vscode#34857 for October

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 91.671% when pulling 1f45dce on mjbvz:meta-embedded-type into 92a639b on Microsoft:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 91.671% when pulling bb79faa on mjbvz:meta-embedded-type into 6ff83ff on Microsoft:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 91.671% when pulling d0b470d on mjbvz:meta-embedded-type into 6ff83ff on Microsoft:master.

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 30, 2017

The changes to use the internal enum type look good to me. Thanks for looking into this

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.

3 participants