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

fix(haskell): Update haskell-tools.nvim to v2.0.0 #553

Merged
merged 8 commits into from
Sep 5, 2023

Conversation

Per48edjes
Copy link
Contributor

@Per48edjes Per48edjes commented Sep 2, 2023

📑 Description

Update the Haskell pack's init.lua to reflect the changes made in mrcjkb/haskell-tools.nvim#227. (The changes herein are lifted directly from the plugin creator's suggested lazy.nvim config.)

ℹ Additional Information

@github-actions
Copy link

github-actions bot commented Sep 2, 2023

Review Checklist

Does this PR follow the [Contribution Guidelines](development guidelines)? Following is a partial checklist:

Proper conventional commit scoping:

  • If you are adding a new plugin, the scope would be the name of the category it is being added into. ex. feat(utility): added noice.nvim plugin

  • If you are modifying a pre-existing plugin or pack, the scope would be the name of the plugin folder. ex. fix(noice-nvim): fix LSP handler error

  • Pull request title has the appropriate conventional commit type and scope where the scope is the name of the pre-existing directory in the project as described above

  • README is properly formatted and uses fenced in links with <url> unless they are inside a [title](url)

  • Proper usage of opts table rather than setting things up with the config function.

lua/astrocommunity/pack/haskell/init.lua Outdated Show resolved Hide resolved
lua/astrocommunity/pack/haskell/init.lua Outdated Show resolved Hide resolved
lua/astrocommunity/pack/haskell/init.lua Outdated Show resolved Hide resolved
@Per48edjes Per48edjes marked this pull request as ready for review September 2, 2023 21:40
@mrcjkb
Copy link
Contributor

mrcjkb commented Sep 2, 2023

Nit: I would suggest changing the PR title's prefix to chore(deps):, as this is not really a bug fix.

@Per48edjes
Copy link
Contributor Author

Nit: I would suggest changing the PR title's prefix to chore(deps):, as this is not really a bug fix.

Fair enough; will update the type to chore but I think this project wants the scope to be the existing enclosing folder.

@Per48edjes Per48edjes changed the title fix(haskell): Update haskell-tools.nvim to v2.0.0 chore(haskell): Update haskell-tools.nvim to v2.0.0 Sep 2, 2023
@Uzaaft
Copy link
Member

Uzaaft commented Sep 3, 2023

@Per48edjes Your first PR title was fine. :)

@Uzaaft Uzaaft changed the title chore(haskell): Update haskell-tools.nvim to v2.0.0 fix(haskell): Update haskell-tools.nvim to v2.0.0 Sep 3, 2023
Copy link
Member

@Uzaaft Uzaaft left a comment

Choose a reason for hiding this comment

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

  1. Add optional to telescope.nvim
  2. I don't think utils.list_insert_unique(astronvim.lsp.skip_setup, "hls") is inplace. It should be
    astronvim.lsp.skip_setup = utils.list_insert_unique(astronvim.lsp.skip_setup, "hls")

@Per48edjes
Copy link
Contributor Author

  1. Add optional to telescope.nvim
  2. I don't think utils.list_insert_unique(astronvim.lsp.skip_setup, "hls") is inplace. It should be
    astronvim.lsp.skip_setup = utils.list_insert_unique(astronvim.lsp.skip_setup, "hls")

Incorporated these in ead0450! Happy to continue moving this forward -- would need some direction on whether this is good as is or whether we're trying to throw vim.g in config.

@mrcjkb
Copy link
Contributor

mrcjkb commented Sep 4, 2023

  1. Add optional to telescope.nvim
  2. I don't think utils.list_insert_unique(astronvim.lsp.skip_setup, "hls") is inplace. It should be
    astronvim.lsp.skip_setup = utils.list_insert_unique(astronvim.lsp.skip_setup, "hls")

Incorporated these in ead0450! Happy to continue moving this forward -- would need some direction on whether this is good as is or whether we're trying to throw vim.g in config.

I don't use AstroNvim, so I don't feel strongly about this.
My recommendation would be to either one of

  • set vim.g.haskell_tools in config (by merging a default with the opts argument - this is fine for a lazily initialized filetype plugin)
  • or incorporate my second suggestion vim.g.haskell_tools = vim.g.haskell_tools or { ... } in init, so that users can override it if they want.

Either way, the previous version of this package was not user-configurable, so I would say go with YAGNI for now.

On a related note: haskell-tools will soon auto-discover DAP configurations by default if nvim-dap is installed and a haskell-debug-adapter executable is detected, see: mrcjkb/haskell-tools.nvim#249
So the call to ht.dap.discover_configurations will be redundant after the 2.1.0 release.

@Uzaaft
Copy link
Member

Uzaaft commented Sep 5, 2023

Appreciate the heads up on the v2.1 launch @mrcjkb

@Uzaaft
Copy link
Member

Uzaaft commented Sep 5, 2023

I'll leave the last word to @mrcjkb He knows his plugin well enough. Any thoughts about where the vim.g code should go, etc?

@mrcjkb
Copy link
Contributor

mrcjkb commented Sep 5, 2023

I'll leave the last word to @mrcjkb He knows his plugin well enough. Any thoughts about where the vim.g code should go, etc?

Let's merge this as is for now and I'll open a separate PR after the 2.1 haskell-tools release 😄

@Uzaaft
Copy link
Member

Uzaaft commented Sep 5, 2023

I'll leave the last word to @mrcjkb He knows his plugin well enough. Any thoughts about where the vim.g code should go, etc?

Let's merge this as is for now and I'll open a separate PR after the 2.1 haskell-tools release 😄

Looking forward to it!

@Uzaaft Uzaaft merged commit ffca0d7 into AstroNvim:main Sep 5, 2023
@Per48edjes
Copy link
Contributor Author

Thanks for all the help @mrcjkb & @Uzaaft ! 🙏

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.

5 participants