-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Simplify for loops in fourslash.ts #11894
Conversation
@@ -1273,8 +1252,8 @@ namespace FourSlash { | |||
if (emitOutput.emitSkipped) { | |||
resultString += "Diagnostics:" + Harness.IO.newLine(); | |||
const diagnostics = ts.getPreEmitDiagnostics(this.languageService.getProgram()); | |||
for (let i = 0, n = diagnostics.length; i < n; i++) { | |||
resultString += " " + diagnostics[0].messageText + Harness.IO.newLine(); |
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.
Assuming this should have been diagnostics[i]
before.
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.
One name change request and I'm not sure whether we're supposed to be using ES5 map.
@@ -124,6 +124,13 @@ namespace ts { | |||
return undefined; | |||
} | |||
|
|||
export function zip<T, U>(arrayA: T[], arrayB: U[], callback: (a: T, b: U, index: number) => void): void { |
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.
lodash and ramda call this zipWith
, and have zip
with the type (t: T[], u: U[]) => [T,U][]
(as do underscore, F#, Python and Haskell). I'd call it zipWith
|
||
this.raiseError("Member list is not empty at Caret: " + errorMsg); | ||
|
||
this.raiseError(`Member list is not empty at Caret:\nMember List contains: ${stringify(members.entries.map(e => e.name))}`); |
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.
are we using ES5's Array.map now?
|
||
this.raiseError("Completion list is not empty at caret at position " + this.activeFile.fileName + " " + this.currentCaretPosition + errorMsg); | ||
this.raiseError(`Completion list is not empty at caret at position ${this.activeFile.fileName} ${this.currentCaretPosition}\n` + | ||
`Completion List contains: ${stringify(completions.entries.map(e => e.name))}`); |
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.
same here. Are we using Array.map now?
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 think it should be fine since fourslash only runs when testing the compiler, not when actually compiling.
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 can use Array.map
now. We haven't been ES3-compatible for a while.
Majority of changes replace It would make sense to run perf comparisons on various platforms — this change might be suboptimal for the reasons above. |
This is test code -- it won't affect the time running the typescript compiler or services. |
Tests take a considerable time to run too. |
|
No description provided.