Skip to content

Actual signature help trigger filtering#25422

Merged
DanielRosenwasser merged 6 commits into
masterfrom
actualSignatureHelpTriggers
Jul 9, 2018
Merged

Actual signature help trigger filtering#25422
DanielRosenwasser merged 6 commits into
masterfrom
actualSignatureHelpTriggers

Conversation

@DanielRosenwasser
Copy link
Copy Markdown
Member

Fixes #25393.

@DanielRosenwasser DanielRosenwasser changed the title Actual signature help triggers Actual signature help trigger filtering Jul 4, 2018
@DanielRosenwasser DanielRosenwasser force-pushed the actualSignatureHelpTriggers branch from 033aab8 to 9481faa Compare July 4, 2018 00:08
@DanielRosenwasser DanielRosenwasser requested review from a user, mhegazy and sheetalkamat July 5, 2018 23:19
Comment thread src/harness/fourslash.ts
for (const key in options) {
ts.Debug.assert(ts.contains(allKeys, key));
if (!ts.contains(allKeys, key)) {
ts.Debug.fail("Unexpected key " + key);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should return ts.Debug.fail("Unexpected key " + key);, probably.

Copy link
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Jul 9, 2018

Choose a reason for hiding this comment

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

I usually only do that when the compiler needs to know more about reachability, but I don't think it's necessary here unless I'm missing something.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We were talking about making it a lint rule a few weeks ago, that way the reachability graph is more complete and may expose more logic bugs (eg, can a following if statement never actually occur because of the implied throw here).

@DanielRosenwasser DanielRosenwasser merged commit fe2baac into master Jul 9, 2018
@DanielRosenwasser DanielRosenwasser deleted the actualSignatureHelpTriggers branch July 9, 2018 05:09
@DanielRosenwasser
Copy link
Copy Markdown
Member Author

🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants