Update launch to wait for configuration completion before running. #304
Conversation
tsconfig.json
Outdated
| @@ -8,6 +8,7 @@ | |||
| ], | |||
| "sourceMap": true, | |||
| "rootDir": ".", | |||
| "noFallthroughCasesInSwitch": true, | |||
There was a problem hiding this comment.
How is that related to this PR?
There was a problem hiding this comment.
as this PR adds a switch-case I'm not opposed to this if explicit fallthroughs are still allowed (as that's one of the whole points of a switch-case)
There was a problem hiding this comment.
I find things like that strange - a switch case should always fall through if there's no explicit break (maybe I'm to old already).
There was a problem hiding this comment.
LOL, I didn't realize this was going to be so controversial. 😄
as this PR adds a switch-case I'm not opposed to this if explicit fallthroughs are still allowed (as that's one of the whole points of a switch-case)
Yes, you can suppress this check by placing a @ts-ignore or @ts-expect-error comment on the line before the case statement. So in the case you really wanted to fall through, that is possible. I'm not a huge fan of that approach (i.e., using the @ts-ignore or @ts-expect-error) since it's not specific to the fall through check/error, so could mask another errors if they happened to fall on that line as well.
After looking into this a bit more, it seems like this check might be better placed into the lint configuration. I noticed that there is a no-switch-case-fall-through rule available in TSLint, which basically accomplishes the same goal. This seems more configurable as you can adjust the severity level as well as adding a "falls through" comment to specifically allow the fall through condition, without resorting to potentially masking legitimate errors.
I briefly looked at the lint configuration setup for the native-debug extension as it appeared to have been enabled at some point in the past, but I don't believe it is working any longer. From my understanding it was previously integrated with tsc so that the linting would occur during compilation. However, it appears the tsc plugin (tslint-language-service as specified in tsconfig.json) is no longer a devDependency. Thus, it wasn't even installed on my system. Furthermore, it seems like this plugin is deprecated and there may be a newer/different approach these days for the integration of linting into the compilation.
I didn't look into the linter configuration much further, but seems it is no longer automatically invoked as part of the compilation. I ran it from the CLI and noticed I had actually created a few lint violations as part of this change, so I want to clean those up. However, it may be good to create a new issue to cover the broken lint configuration here and just remove the current "noFallthroughCasesInSwitch" for now.
I find things like that strange - a switch case should always fall through if there's no explicit break (maybe I'm to old already).
All this does is potentially trigger a warning or error to be omitted (which could also be suppressed) if such a scenario arises. This allows the developer to confirm their intentions to the compiler/linter that they really want this behavior. It has been my experience (and I had thought common knowledge) that the "C-style" fall through behavior is almost always unintended. In fact I'd guess that a legitimate fall through case occurs orders of magnitude less frequently than accidentally forgetting a break statement (and this check exists in many other linters as well), but maybe your experience has been different than mine.
However, as mentioned above, I'll remove this configuration change, fix my linting issues, and then push an update to this PR. Let me know about the current functionality of the integrated linting and if a new issue should be created for that.
There was a problem hiding this comment.
Let me know about the current functionality of the integrated linting and if a new issue should be created for that.
Sounds reasonable, at least to discuss and track this.
It has been my experience (and I had thought common knowledge) that the "C-style" fall through behavior is almost always unintended. In fact I'd guess that a legitimate fall through case occurs orders of magnitude less frequently than accidentally forgetting a break statement (and this check exists in many other linters as well), but maybe your experience has been different than mine.
I maintain C code where this is intended - and in the places where it is an explicit /* fall-through */ comment is in. "Yes", I think in most cases that wouldn't be intended and possibly a break forgotten, but that's what a switch is. If I don't want that than there's the COBOL EVALUATE statement ;-)
Thanks for the PR cleanup!
|
I haven't tested the result but the description and the changes in general (all but the tsconfig) look reasonable to me. |
Removed the timer which was previously used to trigger execution of the run command in the launch configurations and instead updated the launch configuration to behave like the attach configuration. This now waits until the configurationDoneRequest is received before sending the run command to the debugger. This is expected to address race conditions where breakpoints were not being hit due to the application running prior to the breakpoints having been configured in the debugger.
brownts
force-pushed
the
bugfix/remove_run_timer
branch
from
34659e7
to
866e1ab
Compare
This removes the run timer as discussed in #303.
Removed the timer which was previously used to trigger execution of
the run command in the launch configurations and instead updated the
launch configuration to behave like the attach configuration. This
now waits until the configurationDoneRequest is received before
sending the run command to the debugger. This is expected to address
race conditions where breakpoints were not being hit due to the
application running prior to the breakpoints having been configured in
the debugger.