-
Notifications
You must be signed in to change notification settings - Fork 0
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
Dap ux enhancements #1
Conversation
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.
Other than the eslint errors (which made reviewing harder than necessary), I left a question re: how does this work with existing launch.json
configurations.
src/debugger.ts
Outdated
class NoirDebugConfigurationProvider implements DebugConfigurationProvider { | ||
async resolveDebugConfiguration(folder: WorkspaceFolder | undefined, config: DebugConfiguration, token?: CancellationToken): ProviderResult<DebugConfiguration> { | ||
if (config.program || config.request == 'attach') | ||
return config; |
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.
Shouldn't we run preflight checks for user provided configurations as well? I'm not sure how this works, but if config.program
probably means the configuration comes from the launch.json
file. I think it makes sense to do a preflight using the provided configuration. Otherwise, this only applies to "new" debugging configurations.
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 line is actually a remnant of some earlier tests, but with the final implementation it's effectively dead code, because:
- Our launch json doesn't include define
program
. - We're never attaching to a running debugger.
So I removed this line.
But you're right in that this is ignoring projectFolder
when provided through a launch.json
.
This makes me think though, that we should remove the requirement for projectFolder
from the template launch.json
, because in the most common case you will want it to be discovered based on the file you want to debug. We can still allow people to manually specify tho, so I'll add those tweaks.
Oops sorry about the linting, I didn't know there was a project linter (I should have!) and I didn't stick around the PR long enough to see all those horrible messages 🙈 |
Opening this PR for internal review. Note: it depends on a peer changeset to be merged to the Noir repo (will link once I push that one)