Skip to content
This repository was 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
medwatt wants to merge 1 commit intojose-elias-alvarez:mainfrom
medwatt:vhdl_diagnostic_sources
Open

feat: add diagnostic support for vhdl with vcom and ghdl#1207
medwatt wants to merge 1 commit intojose-elias-alvarez:mainfrom
medwatt:vhdl_diagnostic_sources

Conversation

@medwatt
Copy link
Copy Markdown

@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
Copy Markdown
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" },
Copy link
Copy Markdown
Owner

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" },
Copy link
Copy Markdown
Owner

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" },
Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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