-
Notifications
You must be signed in to change notification settings - Fork 118
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
set strictNullChecks to true in tsconfig.json and fix errors #422
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #422 +/- ##
==========================================
- Coverage 20.47% 20.29% -0.18%
==========================================
Files 14 14
Lines 1807 1818 +11
Branches 392 404 +12
==========================================
- Hits 370 369 -1
- Misses 1392 1402 +10
- Partials 45 47 +2 ☔ View full report in Codecov by Sentry. |
src/backend/mi2/mi2.ts
Outdated
@@ -589,9 +589,9 @@ export class MI2 extends EventEmitter implements IBackend { | |||
let location = ""; | |||
if (breakpoint.countCondition) { | |||
if (breakpoint.countCondition[0] == ">") | |||
location += "-i " + numRegex.exec(breakpoint.countCondition.substring(1))[0] + " "; | |||
location += "-i " + numRegex.exec(breakpoint.countCondition.substring(1))?.[0] ?? "" + " "; |
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 should move the numRegex.exec(...)
into a separate variable, then check that it matches, then either it must match and otherwise print an error and do nothing, or fall back to some value like 0
Since we are using -i
, an argument after it is required and we can't just leave it blank like that. (haven't checked GDB documentation to actually verify this statement though, it just looks like it would work that way)
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.
done
src/backend/mi2/mi2lldb.ts
Outdated
@@ -10,7 +9,7 @@ export class MI2_LLDB extends MI2 { | |||
// Since the CWD is expected to be an absolute path in the debugger's environment, we can test | |||
// that to determine the path type used by the debugger and use the result of that test to | |||
// select the correct API to check whether the target path is an absolute path. | |||
const debuggerPath = path.posix.isAbsolute(cwd) ? path.posix : path.win32; | |||
const debuggerPath = posix.isAbsolute(cwd) ? posix : win32; |
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 looks very weird, I don't think the import separation gains us anything here. In node.js path
used to be a library that you also always just imported as a whole, although I haven't kept up with the JS world there what is used now.
But considering that we are assigning it to debuggerPath
, so it can be used like debuggerPath.join(...)
instead of path.join(...)
to get the correct path
for the current platform, I'd much prefer keeping the path.
prefix
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 reverted all changes to imports.
src/frontend/extension.ts
Outdated
context.subscriptions.push(vscode.commands.registerCommand("code-debug.getFileNameNoExt", () => { | ||
if (!vscode.window.activeTextEditor || !vscode.window.activeTextEditor.document || !vscode.window.activeTextEditor.document.fileName) { | ||
vscode.window.showErrorMessage("No editor with valid file name active"); | ||
export function activate(context: ExtensionContext) { |
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.
keep the vscode
import, it's the recommended style from the vscode docs and also makes finding symbols much easier than global imports. In fact I think all of these imports that you changed are much better in a namespace and shouldn't be unwrapped like this. Especially generic stuff like join
or connect
doesn't make much sense to import on its own and just makes the caller code less readable.
src/gdb.ts
Outdated
@@ -68,8 +69,8 @@ class GDBDebugSession extends MI2DebugSession { | |||
this.started = false; | |||
this.crashed = false; | |||
this.setValuesFormattingMode(args.valuesFormatting); | |||
this.miDebugger.printCalls = !!args.printCalls; | |||
this.miDebugger.debugOutput = !!args.showDevDebugOutput; | |||
this.miDebugger.printCalls = args.printCalls; |
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.
args
is user supplied and may be literally any JSON type. (even though it will show type errors to the user in vscode) Keep the boolean cast to make sure we don't break any type constraints in typescript from here.
src/gdb.ts
Outdated
this.miDebugger.printCalls = !!args.printCalls; | ||
this.miDebugger.debugOutput = !!args.showDevDebugOutput; | ||
this.miDebugger.printCalls = args.printCalls; | ||
this.miDebugger.debugOutput = args.showDevDebugOutput; |
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.
ditto
src/lldb.ts
Outdated
this.miDebugger.printCalls = !!args.printCalls; | ||
this.miDebugger.debugOutput = !!args.showDevDebugOutput; | ||
this.miDebugger.printCalls = args.printCalls; | ||
this.miDebugger.debugOutput = args.showDevDebugOutput; |
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.
ditto
src/lldb.ts
Outdated
this.miDebugger.printCalls = !!args.printCalls; | ||
this.miDebugger.debugOutput = !!args.showDevDebugOutput; | ||
this.miDebugger.printCalls = args.printCalls; | ||
this.miDebugger.debugOutput = args.showDevDebugOutput; |
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.
ditto
src/mago.ts
Outdated
this.miDebugger.printCalls = !!args.printCalls; | ||
this.miDebugger.debugOutput = !!args.showDevDebugOutput; | ||
this.miDebugger.printCalls = args.printCalls; | ||
this.miDebugger.debugOutput = args.showDevDebugOutput; |
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.
ditto
src/mago.ts
Outdated
this.miDebugger.printCalls = !!args.printCalls; | ||
this.miDebugger.debugOutput = !!args.showDevDebugOutput; | ||
this.miDebugger.printCalls = args.printCalls; | ||
this.miDebugger.debugOutput = args.showDevDebugOutput; |
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.
ditto
src/mibase.ts
Outdated
if (!fs.existsSync(systemPath.join(os.tmpdir(), "code-debug-sockets"))) | ||
fs.mkdirSync(systemPath.join(os.tmpdir(), "code-debug-sockets")); | ||
this.commandServer.listen(this.serverPath = systemPath.join(os.tmpdir(), "code-debug-sockets", ("Debug-Instance-" + Math.floor(Math.random() * 36 * 36 * 36 * 36).toString(36)).toLowerCase())); | ||
if (!existsSync(join(tmpdir(), "code-debug-sockets"))) |
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.
keep systemPath
, it's written explicitly to differentiate that this is path
using separators using the host OS config, rather than the debuggerPath, which uses separates on the platform that the debugger is running on
src/mibase.ts
Outdated
@@ -188,7 +188,7 @@ export class MI2DebugSession extends DebugSession { | |||
else | |||
this.miDebugger.stop(); | |||
this.commandServer.close(); | |||
this.commandServer = undefined; | |||
// this.commandServer = undefined; |
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 line
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.
done
src/mibase.ts
Outdated
variablesReference: 0, | ||
result: "" | ||
}; | ||
this.sendResponse(response); |
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.
sendErrorResponse instead of plain sendResponse
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.
done
I fixed everything. |
Is there still something to do to get it accepted? |
Another cleanup. I set
strictNullChecks
totrue
intsconfig.json
and the fixed all the errors.