Skip to content
This repository has been archived by the owner on Aug 12, 2023. It is now read-only.

feat: add diagnostic support for vhdl with vcom and ghdl #1207

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

medwatt
Copy link

@medwatt medwatt commented Oct 23, 2022

No description provided.

@medwatt medwatt changed the title add diagnostic support for vhdl with vcom and ghdl feat: add diagnostic support for vhdl with vcom and ghdl Oct 23, 2022
Copy link
Owner

@jose-elias-alvarez jose-elias-alvarez left a comment

Choose a reason for hiding this comment

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

This looks good overall. I left a couple of comments, and I also wanted to mention that it looks like there is a ghdl language server and it's supported by nvim-lspconfig. I know nothing about this ecosystem, but if that works, it may be an easier solution, since actual LSP servers will provide a better experience than null-ls.

to_stdin = false,
to_temp_file = true,
from_stderr = true,
args = { "-a", "--std=08", "--workdir=/tmp", "$FILENAME" },

Choose a reason for hiding this comment

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

I know nothing about this language or CLI tool, but judging from the documentation, it looks like there are multiple standards. Is this something we want to always set, or should we leave it up to users to configure?

to_stdin = false,
to_temp_file = true,
from_stderr = true,
args = { "-a", "--std=08", "--workdir=/tmp", "$FILENAME" },

Choose a reason for hiding this comment

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

Also, just so I understand: is the --workdir flag necessary because the program will otherwise compile and leave files in the current directory? If this is necessary then we may want to mention that the source will not work as expected on platforms that don't use /tmp.

filetypes = { "vhdl" },
generator_opts = {
command = "vcom",
args = { "-2008", "-quiet", "-lint", "-work", "/tmp/vcom_work", "$FILENAME" },

Choose a reason for hiding this comment

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

Same comments for above apply here.

@medwatt
Copy link
Author

medwatt commented Oct 24, 2022

@jose-elias-alvarez I use mason.nvim, which is supposed to be the successor for nvim-lspconfig. I didn't see any reference to ghdl.

@jose-elias-alvarez
Copy link
Owner

I don't use mason.nvim, but I don't think it's supposed to replace nvim-lspconfig. nvim-lspconfig's role is to configure language servers, while (as I understand it) Mason's role is to manage the installation of language servers and other tools.

Before moving forward with this PR, it would be good to confirm whether the language server provides the same functionality.

@medwatt
Copy link
Author

medwatt commented Oct 25, 2022

I don't use mason.nvim, but I don't think it's supposed to replace nvim-lspconfig. nvim-lspconfig's role is to configure language servers, while (as I understand it) Mason's role is to manage the installation of language servers and other tools.

Before moving forward with this PR, it would be good to confirm whether the language server provides the same functionality.

Sorry for that. I think I confused nvim-lspconfig with nvim lsp installer. I don't use nvim-lspconfig directly, so I wouldn't know if the language server provides the same functionality. I user mason to do all the configuration / downloading of the lsp servers.

@jose-elias-alvarez
Copy link
Owner

That's fine then, let me know what you think about the other comments I left. There's also a failing CI step from stylua.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants