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

SQL Server typos and keyword completion modifier #2460

Merged
merged 14 commits into from Apr 23, 2015
Merged

SQL Server typos and keyword completion modifier #2460

merged 14 commits into from Apr 23, 2015

Conversation

Sevin777
Copy link
Contributor

This contains a few different things:

  1. Fixed a few of typos in files related SQL Server mode
  2. Added a few snippets and enhanced others for SQL Server mode
  3. Override getCompletions for SQL Server mode to ensure proper case of completions and use intelligent meta instead of keyword for all completions.

image

3. Added completionModifier to text_highlight_rules, which is an optional function that can be specified by a mode's highlight rules that inherit from text_highlight_rules.
4. Modified getCompletions in text mode to call the completionModifier if it exists.

The completion modifier allows modifying keyword completions, which is useful for a few things:
- Remove keywords that are obsolete from the completor, but they still need to have syntax highlighting
- Modify the meta display in the completer to provide more information than keyword for some things
- Modify the case of completions, use case: SQL Server mode has completions that should be all caps, and others that should not (even though all keywords are case-insensitive for highlighting, its simply recommended by Microsoft to use all caps for most keywords).

I modified SQL Server mode to use the completion modifier, and I went ahead and modified Javascript mode as well.

If you don't like the completion modifier and the javascript modifications, I can remove them and simply do a one off for SQL Server mode completions by overriding getCompletions for SQL Server mode. I decided to make the completion modifier standard because I think it could benefit almost all modes.

Javascript mode completion popup with modified meta for keywords:

image

@nightwing
Copy link
Member

I like the result of using completionModifier, however having it called from getCompletions doesn't seem to simplify the code, that is overriding getCompletions would require same amount of code which will work a bit faster. Also we could use information about obsolete keywords from the original keyword map instead of throwing it out at https://github.com/ajaxorg/ace/blob/master/lib/ace/mode/text_highlight_rules.js#L220

@Sevin777
Copy link
Contributor Author

Good point. I think it would be valuable to have a standardized way to have create more intelligent keyword completions for each mode, but I don't have the time to figure out the best implementation.

I removed all changes that are not for SQL Server mode and added an override for getCompletions for SQL Server mode only. I am caching the result of this function as it takes ~15ms to execute (compared to ~1ms when there is no extra filtering). I don't see any harm in caching the result, do you?

UPDATE
I realized I can't cache the result of getCompletions because the filtering of the list references the cached result, which messes up subsequent calls. Caching would require a deep clone, which would be only marginally faster than not caching. I don't think there is an issue with waiting ~15ms to get the completions (Tern commonly waits ~1,000ms)

was using wrong escape char for string, sqlserver uses double single
quote to escape a single quote
@nightwing
Copy link
Member

Are you sure about caching? autocompleter doesn't modify array of matches it receives from getCompletions https://github.com/ajaxorg/ace/blob/master/lib/ace/autocomplete.js#L237.

var keywords = this.$keywordList || this.$createKeywordList();
var types = session.$mode.$highlightRules.keywordTypes;

return keywords.map(function(word) {
Copy link
Member

Choose a reason for hiding this comment

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

could we just use types skipping keywordList entirely?
keywordList removes some information that highlighter doesn't need and here we are restoring that using slow indexOf in a large array

@Sevin777
Copy link
Contributor Author

Good point about skipping the keyword list, the custom completions is now simpler and its only compiled once.

@@ -164,14 +154,11 @@ var SqlServerHighlightRules = function() {
start: "/\\*",
end: "\\*/"
}, {
token: "string", // ' string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was the unnecessary rule you were referring to, correct? I think I copied that rule from somewhere else

nightwing added a commit that referenced this pull request Apr 23, 2015
SQL Server typos and keyword completion modifier
@nightwing nightwing merged commit 65ad040 into ajaxorg:master Apr 23, 2015
@nightwing
Copy link
Member

Merged.
Thanks!

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