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

feat: add dap subcommand and support basic DAP features #349

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

rinx
Copy link
Contributor

@rinx rinx commented Sep 23, 2024

This pull request adds dap subcommand and basic Debug Adapter Protocol (DAP) features.

Falco already has a VCL debugger.
It's better to integrate DAP features with it, however the current debugger is highly coupled with TUI console.
Not to break compatibility, at the first step, I implemented DAP features as separated subcommand.

Demo

In my Neovim environment, it works like below:
falco-dap-demo

that contains basic functions to handle dap requests

https://microsoft.github.io/debug-adapter-protocol/specification

Signed-off-by: Rintaro Okamura <[email protected]>
to launch dap server

Signed-off-by: Rintaro Okamura <[email protected]>
rinx added a commit to rinx/dotfiles that referenced this pull request Sep 23, 2024
falco's dap feature is proposed in ysugimoto/falco#349

Signed-off-by: Rintaro Okamura <[email protected]>
Signed-off-by: Rintaro Okamura <[email protected]>
Signed-off-by: Rintaro Okamura <[email protected]>
@ysugimoto
Copy link
Owner

@rinx
Thanks for your great implementation!
It seems to be enough for the begging of DAP support.
I will look into the implementation deeply, bear with my review.

Copy link
Owner

@ysugimoto ysugimoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, and put one comment for getting breakpoint.

[TODO] We need to write documentation for DAP usage, should be done in another PR.

dap/debugger.go Outdated

path, err := filepath.Abs(meta.Token.File)
if err != nil {
// TODO: handle error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should we treat for the error?
In terms of ast.Node, falco always treat as absolute path so meta.Token.File always has an absolute path.
Therefore you don't need to get the absolute path using filepath.Abs.

Additionally, falco may retrieves Fastly remote reources like acl, backend, etc then the remote source file will be Remote.Acl.[name] and the the getBreakpoint` works fine even not a filepath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your comment. I've just fixed it!

Additionally, falco may retrieves Fastly remote reources like acl, backend, etc then the remote source file will be Remote.Acl.[name] and the the getBreakpoint` works fine even not a filepath?

Currently, it doesn't matter because dap subcommand does not support remote resources feature.
We need to implement it as a future work.

Signed-off-by: Rintaro Okamura <[email protected]>
Copy link
Owner

@ysugimoto ysugimoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This should be merged.

@ysugimoto ysugimoto merged commit 12fd8b0 into ysugimoto:main Oct 14, 2024
1 check 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