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

refactor(dap): split dap and dap_ui integrations #624

Merged
merged 11 commits into from
Dec 8, 2023
Merged

refactor(dap): split dap and dap_ui integrations #624

merged 11 commits into from
Dec 8, 2023

Conversation

igorlfs
Copy link
Contributor

@igorlfs igorlfs commented Dec 7, 2023

This Pull Request makes 3 changes to the nvim-dap integration:

  1. Splits the dap and dap_ui integrations. Rationale: there's no need for both plugins to share a integration. In fact, that's the only integration that contains multiple plugins.
  2. Adds a missing highlight group to the nvim-dap integration
  3. Removes a misguided comment about enabling the nvim-dap integration

I'm not sure why, but the integrations are not enabled by default. Did I forget something?

This Pull Request makes 3 changes to the nvim-dap integration:
1. Splits the dap and dap_ui integrations. Rationale: there's no need for
   both plugins to share a integration. In fact, that's the only
   integration that contains multiple plugins.
2. Adds a missing highlight group to the nvim-dap integration
3. Removes a misguided comment about enabling the nvim-dap integration
@igorlfs igorlfs changed the title refactor(dap)!: split dap and dap_ui refactor(dap)!: split dap and dap_ui integrations Dec 7, 2023
@mrtnvgr
Copy link
Contributor

mrtnvgr commented Dec 7, 2023

Hey, thanks for your pull request. There are some things you forgot, you should:

  • Update integration types in lua/catppuccin/types.lua
  • Update selene integration
  • ...

I'm busy right now, will take a look a bit later.

@mrtnvgr
Copy link
Contributor

mrtnvgr commented Dec 7, 2023

I'm not sure why, but the integrations are not enabled by default.

All integrations are disabled by default, only some of them are enabled in default_options here.

I wonder why 🤔

Other than that, everything looks nice, @igorlfs can I merge this?

@nullchilly
Copy link
Contributor

nullchilly commented Dec 7, 2023

  1. Need to add backwards compatibility because we're no longer 0ver (We're pretty much against unnecessary breaking changes)
dap = { enabled = true, dap_ui = true }

should be auto converted to

dap = true, dap_ui = true

Something like this would work (Please verify very carefully):

if O.integrations.dap.enabled and O.integrations.dap.dap_ui ~= nil then
  O.integrations.dap_ui = O.integrations.dap.dap_ui
end
  1. Integrations are enabled by default if they're known to be well tested.

@nullchilly
Copy link
Contributor

Failed on require("catppuccin").setup { integrations = { dap = true } }

Failed to run `config` for catppuccin

...chilly/code/git/catppuccin/lua/catppuccin/lib/mapper.lua:30: attempt to index field
'dap' (a boolean value)

@nullchilly nullchilly changed the title refactor(dap)!: split dap and dap_ui integrations refactor(dap): split dap and dap_ui integrations Dec 8, 2023
@nullchilly nullchilly merged commit d5c07c0 into catppuccin:main Dec 8, 2023
9 checks passed
@igorlfs igorlfs deleted the refactor/split-dap-and-dap-ui branch December 8, 2023 15:06
@igorlfs
Copy link
Contributor Author

igorlfs commented Dec 8, 2023

Hey! Thanks for merging! I've been kinda busy lately, so I couldn't catch up!

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