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

[JavaScript] Use prototype for comments #1007

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

Thom1729
Copy link
Collaborator

(This PR formerly known as #1003)

Background

The default JavaScript syntax definition is descended from an old .tmLanguage. Since its transition to a .sublime-syntax, it has largely modernized, but signs remain of its history.

One such sign is that the syntax definition does not use the new prototype feature for comments. Instead, the comments context is included manually. There are many dozens of contexts in the definition and somewhere between many and most of them should have manually included comments. An unknown number of those did not, resulting in at least one bug (#885, first test case).

In addition, the tests did not include any negative tests to ensure that comments were not included where they should not have been. No related bugs are known.

This commit

I have moved the comments include to the prototype and removed it from all other contexts. I added meta_include_prototype: false as appropriate to remove it from the few contexts that should not include it. Finally, I have added negative tests to verify that comments should not appear in those contexts and a test for the known bug that was fixed in the process.

Discussion

The known bug could have been fixed by adding another manual include. So why make a larger change?

The primary reason is simplicity. Comments are a pervasive feature that applies to nearly every part of the language. Each - include: comments is a separate implementation of this feature. While not every context would need that include, it is necessary for every context to consider whether it is needed. In at least one known case, the include was mistakenly omitted (and there are probably more such cases).

Conversely, the parts of the language that do not need to support comments are few: strings, regexps, and comments themselves. These parts all stand out as special cases with their own unique microsyntaxes. Additionally, this enumeration is likely to be relatively stable as the language evolves. Every piece of new syntax may require a new context and explicit consideration of comments, whereas new types of strings don't come along very often.

Prototypes also lessen the testing requirements. In this commit, I added a test for each meta_include_prototype: false that covers all comment types. Without prototypes, there really ought to be such a test for every single context that does allow comments. Many, many tests would be required to catch bugs such as the example from #885. Such tests do not yet exist.

In short, I believe that it makes the most sense to mark each exception rather than each non-exception. Doing so will result in less code and require fewer tests.

Remarks

@wbond
Copy link
Member

wbond commented Jun 8, 2017

Thank you – I appreciate the explanation of what and why.

@wbond wbond merged commit 249b650 into sublimehq:master Jun 8, 2017
@Thom1729 Thom1729 deleted the javascipt-prototype-comments branch December 6, 2017 15:53
deathaxe pushed a commit to deathaxe/sublime-packages that referenced this pull request Jun 9, 2019
…omments

[JavaScript] Use prototype for comments
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.

2 participants