-
Notifications
You must be signed in to change notification settings - Fork 5
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
Features/add feature flags #12
base: master
Are you sure you want to change the base?
Conversation
Add .eslintrc for my sanity
…ite number of features. Moved `Context.language` check in 1-1 error to `Context.features.binarySubtractRequiresLeadingWhitespace` Fix up a few things after looking around
… entail. Fix issues where textDocument/reference would return bad symbols and cause exceptions; "null" is a valid return value for this event, so we use that now instead (-1 as line/col was invalid, too). Push (end) token in the token list, so that "second-from-last" resolves (on missingSemicolon for instance) end up being handled.
server/src/language.js
Outdated
end: { line: -1, character: -1} | ||
} | ||
}; | ||
return null; |
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.
This fixes two different bugs. The language server accepts "null" as meaning "no location"; -1 causes it to throw an exception.
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.
This happens if we are trying to get the Program for a document we don't know about. How are we getting into this case?
# Conflicts: # parser/quakec-parser.js # server/src/language.js
# Conflicts: # parser/quakec-parser.js # server/src/language.js
This branch depends on the stability branch now, so once that's ready we'll re-review this. |
# Conflicts: # parser/quakec-parser.js # server/src/language.js
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.
This is looking great! Requested some feedback on naming, verbiage, and other style stuff.
server/src/language.js
Outdated
end: { line: -1, character: -1} | ||
} | ||
}; | ||
return null; |
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.
This happens if we are trying to get the Program for a document we don't know about. How are we getting into this case?
parser/quakec-parser.js
Outdated
} | ||
}); | ||
|
||
handler(Feature.supported(languageFeature)); |
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.
I like passing whether the feature is supported into the call back. 👍
parser/quakec-parser.js
Outdated
* @param {LanguageFeatures} languageFeature | ||
* @return {boolean} | ||
*/ | ||
static supported(languageFeature) { |
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.
Both supported
and unsupported
probably shouldn't be used outside of Feature
? Should we rename them to _supported
and _unsupported
? I'd like to hear your thoughts.
parser/quakec-parser.js
Outdated
@@ -1444,6 +1503,7 @@ class Program { | |||
* | |||
* @param {Position} position | |||
* @param {boolean} includeDeclaration | |||
* @param {boolean} recursive |
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.
Remove this.
parser/quakec-parser.js
Outdated
* @param {LanguageFeatures} languageFeature | ||
* @return {boolean} | ||
*/ | ||
static unsupported(languageFeature) { |
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 might not need this.
parser/quakec-parser.js
Outdated
@@ -1022,6 +1079,7 @@ Define.symbol("["); | |||
Define.symbol("]"); | |||
Define.symbol("$"); | |||
Define.symbol("else"); | |||
Define.symbol(":"); |
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.
Remove.
This includes several parser fixes as well as the introduction of feature flags and a test of said feature flags with the a ? b : c and a ?: b operators.