Skip to content
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

Merged
merged 6 commits into from
Jan 23, 2024
Merged

Dap ux enhancements #1

merged 6 commits into from
Jan 23, 2024

Conversation

mverzilli
Copy link

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)

@mverzilli mverzilli requested a review from ggiraldez January 18, 2024 14:34
Copy link
Member

@ggiraldez ggiraldez left a 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;
Copy link
Member

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.

Copy link
Author

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:

  1. Our launch json doesn't include define program.
  2. 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.

@mverzilli
Copy link
Author

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.

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 🙈

@mverzilli mverzilli marked this pull request as ready for review January 23, 2024 16:41
@mverzilli mverzilli merged commit d88bc0b into dap-support Jan 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants