-
Notifications
You must be signed in to change notification settings - Fork 5
Compass 2934 don not autocomplete in comments #1
Conversation
test/stage-autocompleter.test.js
Outdated
}); | ||
}); | ||
|
||
context('symbol before comment on the beginning of editor', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get one more test for multiline please? Something where the session is new EditSession('/**\n * query - The query in MQL.\n */ {}', new Mode());
and the position is { row: 1, column: 5 }
?
Nice job - well tested. I think just the one multiline test to add and this is good to go. :) |
lib/stage-autocompleter.js
Outdated
value.slice(item.index + item[0].length) | ||
].join(''); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have input like this:
{x: 'uuu//UU'} // $
we need to check if ´//´ is a real start tag of the comment, or it can be a part of a string. I couldn't create a single regex to handle it, therefore i replace stings like this which contain //
with {x: #########} // $
and after it i can find real comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great - just a few more changes so we are compatible with IE and for performance.
lib/stage-autocompleter.js
Outdated
@@ -121,9 +121,42 @@ class StageAutoCompleter { | |||
* | |||
* @returns {Function} The completion function. | |||
*/ | |||
getCompletions(editor, session, position, prefix, done) { | |||
async getCompletions(editor, session, position, prefix, done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use async/await as this will not be compatible with Cloud. They need to support IE.
lib/stage-autocompleter.js
Outdated
|
||
// Comments block do not return results. | ||
let value = session.getValue(); | ||
const findCommentsRe = new RegExp([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating the regex every time the hints are requested, I would move this to a constant so it's only created once at parse time.
lib/stage-autocompleter.js
Outdated
|
||
const activePosition = session.getDocument().positionToIndex(position, 0); | ||
let isComment = false; | ||
const findCommnets = () => new Promise((resolve) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promises are also not supported in IE. Also typo here, should be findComments
. But let's stick with standard callbacks for the autocomplete.
Don't do autocompletion when typing inside comments in the stage.