Adding support for multiple lineComment prefixes #3121
Conversation
ghost
assigned 
|
To @redmunds |
|
Two quick thoughts:
|
… and adding new test
|
I made setLineCommentSyntax take a string or an array of strings and added a new test in LanguageManager. I was thinking of adding some tests to EditorCommandHandlers, but it would require to change the JavaScript or CSS line comment syntax for those tests, or I should do it in PHP mode, but could be broken if PHP is removed from the core? |
| function _matchExpressions(string, expressions) { | ||
| var matchAny = false; | ||
| expressions.forEach(function (exp) { | ||
| matchAny = matchAny || string.match(exp); |
There was a problem hiding this comment.
This loop searches for every expression no matter what. It should stop searching after a match is found.
|
Done with initial review. |
| */ | ||
| function _getLinePrefix(string, expressions, prefixes) { | ||
| var result = null; | ||
| expressions.forEach(function (exp, index) { |
There was a problem hiding this comment.
Is needed here to cover the entire array to cover cases where the line prefixes are prefixes of each other and find the best match. Peter mentioned a case in #3056 where in Clojure people use ; and ;; for line commenting, and this should cover that case.
|
@redmunds Thanks for the review. I fixed your comments, except the ones I commented on, so please read my replies and let me know what should be done. Thanks. |
| // A line is commented out if it starts with 0-N whitespace chars, then "//" | ||
| if (!line.match(lineExp) && line.match(/\S/)) { | ||
| // A line is commented out if it starts with 0-N whitespace chars, then a line comment prefix | ||
| console.log(_matchExpressions(line, lineExp)); |
There was a problem hiding this comment.
Need to remove this call to console.log().
|
Uncommenting is not working correctly in a .js file (Mac 10.8). I select 3 lines and press Cmd-/ and the lines are commented with line comments. Selecting the same 3 lines an pressing Cmd-/ again, only the first line is uncommented. |
|
Done with second review. |
|
Oops, I left an unwanted break from the previews commit. It should work for every line now. I added a couple of new tests for I am not sure if I like the change of |
Seems like it's easier to always pass empty arrays everywhere and not have to check for null anywhere. Why not just fix the places that pass null/undefined? |
| * Returns the prefix to use for line comments. | ||
| * @return {string} The prefix | ||
| * Returns an array of prefixes to use for line comments. | ||
| * @return {Array.<string>} The prefixes | ||
| */ | ||
| Language.prototype.getLineCommentPrefix = function () { |
There was a problem hiding this comment.
This should be changed to should be changed to getLineCommentPrefixes now that there can be multiple.
| @@ -449,26 +450,37 @@ define(function (require, exports, module) { | |||
| * @return {boolean} Whether line comments are supported | |||
| */ | |||
| Language.prototype.hasLineCommentSyntax = function () { | |||
| return Boolean(this._lineCommentSyntax); | |||
| return Boolean(this._lineCommentSyntax.length); | |||
There was a problem hiding this comment.
The intent of the code would be more clear to return a boolean expression than to convert an integer to Boolean type:
return (this._lineCommentSyntax.length > 0);
|
Done with review. |
|
@redmunds all done. |
|
Looks good. Merging. |
Adding support for multiple lineComment prefixes
|
Great. Thanks :) |
There are a few languages like PHP that support 2 types of line comment ("//" and "#").
To address this, I made lineComment be an array for every language and saved also as an array in Language. It will also return an array when asking for a linePrefix.
lineCommentPrefixcan work with multiple line prefixes and uncomment any line comment prefix defined in the array if it is found at the start of the line as before, but it will always comment using the first line comment prefix.blockCommentPrefixSuffixcan also work multiple line prefixes and uncomment any line comment prefix defined in the array. This also fixes a bug where in PHP if there was a comment using#and you would Toggle Block Comment inside that line comment it could uncomment any block comment found before that line.