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

plugins/dap: migrate to mkNeovimPlugin #2897

Merged
merged 7 commits into from
Jan 27, 2025
Merged

Conversation

khaneliman
Copy link
Contributor

No description provided.

@khaneliman khaneliman force-pushed the dap branch 5 times, most recently from 00fac7c to 690a920 Compare January 26, 2025 21:15
@khaneliman khaneliman marked this pull request as ready for review January 26, 2025 21:30
@khaneliman khaneliman requested a review from a team January 26, 2025 21:30
Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

I've not cross-referenced the old/new impl. Nor am I familiar with DAP in general. But on the whole everything looks good.

I've left some comments below, mostly on existing code that could be improved as part of this PR.

plugins/by-name/dap-python/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap-python/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap-python/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap-python/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap-ui/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap-virtual-text/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap/default.nix Outdated Show resolved Hide resolved
plugins/by-name/dap-go/default.nix Outdated Show resolved Hide resolved
@khaneliman khaneliman force-pushed the dap branch 5 times, most recently from 765b08c to 9b80a74 Compare January 26, 2025 22:35
@khaneliman khaneliman force-pushed the dap branch 4 times, most recently from 7b5dc4a to 30c289b Compare January 27, 2025 00:02
Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

I'm not too confident with this part of the codebase, but I don't see any issues.

I'd feel better if @GaetanLepage or @traxys could also review before merging, assuming they have time.

@khaneliman
Copy link
Contributor Author

khaneliman commented Jan 27, 2025

Was testing with this branch for a bit and then wanted to test lazy loading. Had a couple tweaks for the packPathNames and the final generated config for dap

image

@khaneliman
Copy link
Contributor Author

Alright, lazy loading is working with this PR.

image vs image


clear_on_continue = defaultNullOpts.mkBool false "Clear virtual text on `continue` (might cause flickering when stepping).";

display_callback = defaultNullOpts.mkLuaFn ''
Copy link
Member

Choose a reason for hiding this comment

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

I would rather drop mkLuaFn in favor of mkRaw.

Copy link
Member

Choose a reason for hiding this comment

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

Bearing in mind this is a migration, wouldn't that be a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally left these alone.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Ok for me then !


customConfigurations = mkNullOrOption (types.listOf dapHelpers.configurationOption) "Custom python configurations for dap.";

resolvePython = defaultNullOpts.mkLuaFn null ''
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I would rather go for mkRaw.

Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Good job !

@khaneliman
Copy link
Contributor Author

@mergify queue

Copy link
Contributor

mergify bot commented Jan 27, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 32aa73a

Copy link
Contributor

mergify bot commented Jan 27, 2025

This pull request, with head sha 32aa73af4742dce6532abe7a181370290fe8a727, has been successfully merged with fast-forward by Mergify.

This pull request will be automatically closed by GitHub.

As soon as GitHub detects that the sha 32aa73af4742dce6532abe7a181370290fe8a727 is part of the main branch, it will mark this pull request as merged.

It is possible for this pull request to remain open if this detection does not happen, this usually happens when a force-push is done on this branch dap, this means GitHub will fail to detect the merge.

@mergify mergify bot merged commit 32aa73a into nix-community:main Jan 27, 2025
4 checks passed
@mergify mergify bot temporarily deployed to github-pages January 27, 2025 16:16 Inactive
@khaneliman khaneliman deleted the dap branch January 27, 2025 16:17
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.

3 participants