feat: set cwd for dlv based on program #99

2024-09-22T16:18:23+03:00

mcoqzeug commented

2024-09-22 16:18:23 +03:00

(Migrated from github.com)

This potentially resolves #85.

Similar to vscode-go, set dlv.executable.cwd based on the value of debug_config.program. If program is a directory, set cwd to program. If program is a path to a go file, set cwd to its parent directory.
Convert program to absolute path.

We probably don't need config.delve.cwd anymore.

Reference: https://github.com/golang/vscode-go/blob/master/extension/src/goDebugConfiguration.ts#L545

This potentially resolves #85. - Similar to vscode-go, set `dlv.executable.cwd` based on the value of `debug_config.program`. If `program` is a directory, set `cwd` to `program`. If `program` is a path to a go file, set `cwd` to its parent directory. - Convert `program` to absolute path. We probably don't need `config.delve.cwd` anymore. Reference: https://github.com/golang/vscode-go/blob/master/extension/src/goDebugConfiguration.ts#L545

leoluz (Migrated from github.com) requested changes

2024-10-05 07:19:17 +03:00

lua/dap-go.lua

					
				@ -81,6 +81,24 @@ local function setup_delve_adapter(dap, config)

				  }

This would introduce an external dependency to plenary. I prefer to keep the dependencies in this plugin as small as possible.

This would introduce an external dependency to `plenary`. I prefer to keep the dependencies in this plugin as small as possible.

mcoqzeug (Migrated from github.com) reviewed

2024-10-05 19:45:48 +03:00

lua/dap-go.lua

					
				@ -81,6 +81,24 @@ local function setup_delve_adapter(dap, config)

				  }

Hi @leoluz, thanks for the feedback. I've replaced plenary.path with vim.loop functions.

Hi @leoluz, thanks for the feedback. I've replaced `plenary.path` with `vim.loop` functions.

leoluz (Migrated from github.com) reviewed

2024-10-17 04:28:24 +03:00

lua/dap-go.lua

					
				@ -81,6 +81,24 @@ local function setup_delve_adapter(dap, config)

				  }

Thank you

leoluz (Migrated from github.com) requested changes

2024-10-17 04:32:55 +03:00

leoluz (Migrated from github.com) left a comment

@mcoqzeug Thank you for removing plenary dependency.
I noticed that you are re-implementing file path logic that is already available as vim functions. The get_parent function for example can be replaced by fnamemodify (see https://stackoverflow.com/questions/16485748/how-to-get-the-parent-directory-of-a-path-string). Similar strategy can be used to extract the file extension with filename-modifier :e (see https://vimhelp.org/cmdline.txt.html#filename-modifiers)

@mcoqzeug Thank you for removing plenary dependency. I noticed that you are re-implementing file path logic that is already available as vim functions. The `get_parent` function for example can be replaced by `fnamemodify` (see https://stackoverflow.com/questions/16485748/how-to-get-the-parent-directory-of-a-path-string). Similar strategy can be used to extract the file extension with filename-modifier `:e` (see https://vimhelp.org/cmdline.txt.html#filename-modifiers)

TendonT52 commented

2025-01-07 16:05:15 +03:00

(Migrated from github.com)

Thank you, I'm just using this branch, and it works. Hope this will merge soon.

leoluz (Migrated from github.com) requested changes

2025-02-03 06:18:24 +03:00

leoluz (Migrated from github.com) left a comment

Hi @mcoqzeug .
Thank you for the latest changes and fixes. PR is looking much cleaner now.
To proceed with the merge, the PR would need to include documentation updates in the README.md as well as in the doc/[nvim-dap-go.txt files
Thanks again!

Hi @mcoqzeug . Thank you for the latest changes and fixes. PR is looking much cleaner now. To proceed with the merge, the PR would need to include documentation updates in the `README.md` as well as in the `doc/[nvim-dap-go.txt` files Thanks again!

jinzhongjia commented

2025-03-10 19:07:01 +03:00

(Migrated from github.com)

why not merge this? then update docs?

mcoqzeug commented

2025-03-10 20:33:06 +03:00

(Migrated from github.com)

Hi @mcoqzeug . Thank you for the latest changes and fixes. PR is looking much cleaner now. To proceed with the merge, the PR would need to include documentation updates in the README.md as well as in the doc/[nvim-dap-go.txt files Thanks again!

Hi @leoluz, I think this PR only changes a small implementation detail and is compatible with existing documentation. The only change I can think of is to remove the cwd option from REAME.md: https://github.com/leoluz/nvim-dap-go/blob/main/README.md?plain=1#L90-L92. What do you think?

> Hi @mcoqzeug . Thank you for the latest changes and fixes. PR is looking much cleaner now. To proceed with the merge, the PR would need to include documentation updates in the `README.md` as well as in the `doc/[nvim-dap-go.txt` files Thanks again! Hi @leoluz, I think this PR only changes a small implementation detail and is compatible with existing documentation. The only change I can think of is to remove the `cwd` option from `REAME.md`: https://github.com/leoluz/nvim-dap-go/blob/main/README.md?plain=1#L90-L92. What do you think?

kaldius (Migrated from github.com) reviewed

2025-03-24 07:59:49 +03:00

lua/dap-go.lua

					
				@ -81,6 +81,24 @@ local function setup_delve_adapter(dap, config)

				  }

Hi, @mcoqzeug I'm facing the same issue and your fork almost solved it for me, so thank you so much for this. I did notice that on my machine, :lua print(vim.fn.fnamemodify("/a/b/c/main.go", ":e")) returns "go" rather than ".go", so deleting the . in this line was the final step to get it working for me.

Wondering if it's the same on your side?

Thanks again for your efforts and hope this PR gets merged soon!

Hi, @mcoqzeug I'm facing the same issue and your fork almost solved it for me, so thank you so much for this. I did notice that on my machine, `:lua print(vim.fn.fnamemodify("/a/b/c/main.go", ":e"))` returns `"go"` rather than `".go"`, so deleting the `.` in this line was the final step to get it working for me. Wondering if it's the same on your side? Thanks again for your efforts and hope this PR gets merged soon!

carlosflorencio commented

2025-05-13 23:35:50 +03:00

(Migrated from github.com)

Also created https://github.com/leoluz/nvim-dap-go/pull/121 to fix this issue, unfortunately I've only noticed this PR afterwards.

dennypenta commented

2025-08-09 23:37:50 +03:00

(Migrated from github.com)

@leoluz does only missing documentation blocks us from merging it?

stale[bot] commented

2025-11-10 06:21:37 +03:00

(Migrated from github.com)

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin mcoqzeug/set-cwd-for-dlv:mcoqzeug/set-cwd-for-dlv

git switch mcoqzeug/set-cwd-for-dlv

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff mcoqzeug/set-cwd-for-dlv

git switch mcoqzeug/set-cwd-for-dlv

git rebase main

git switch main

git merge --ff-only mcoqzeug/set-cwd-for-dlv

git switch mcoqzeug/set-cwd-for-dlv

git rebase main

git switch main

git merge --no-ff mcoqzeug/set-cwd-for-dlv

git switch main

git merge --squash mcoqzeug/set-cwd-for-dlv