-
-
Notifications
You must be signed in to change notification settings - Fork 189
Added Tex Language Support #569
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
base: main
Are you sure you want to change the base?
Conversation
|
This should satisfy #525 |
|
Feature essentially done. Implemented:
Not implemented:
Due to the scale that this language implementation ended up being I organized it differently compared to the other languages; it is in multiple files and modules, all called by a I have not tested every single combination of options as that would take forever. I have tested many of the more relevant ones but I have not tested the texlab experimental settings for instance. The config I am currently using is as follows: I have created frameworks for easily adding both new pdf viewers and new tex builders/compilers. Adding other LSP servers should be not too complicated if texlab proves to be fundamentally inadequate. Overall, this implementation will cover all of my needs for tex and includes many other features I will likely not use but have been implemented to achieve near feature parity with many of the implemented programs which will hopefully cover most of what anyone else requires. If not, in a few places I added methods to be able to add custom configuration just in case. There are no other features I plan to add for the time being, but I will try to fix bugs as they come up on my side. |
|
Looks good from a first glance. Thank you for your hard work. I'm too tired to review in full at the moment, but I'll properly review tomorrow when I wake up. One thing that comes to mid is that |
|
If you could also fix the Cı errors by then, that'd be great. |
|
Sounds good, I’ll get onto those things when I next get a moment |
|
I made some of the changes you requested. I made a mkEnableTreesitterOption function in the extended library. I must say I'm not too familiar with extending lib so it's a little different from how the original function was. I didn't want to start messing with the imports of the existing lib extensions by finding a way to add config as an input so the function itself requires passing the config to it as one of its inputs. If there's a better/cleaner way to do this feel free to make those changes or provide a source where I can learn how to do it properly. If there are any other changes you wish to be made don't hesitate to let me know. On an aside, I have been testing this PR this whole time for academic work and I can now also say that as I have now started using bibtex I can say that it has been working flawlessly and multi-file build and re-building has been working great too. |
|
I'm going to have to ask you to rebase. With #605 we have transitioned to an alternative method that is faster and cheaper than flake inputs for plugins, which does meant that there will be merge conflicts I will need you to fix before I can merge this. Meanwhile, I'll provide a complete review, there shouldn't be too much to cover this time around. |
|
I was wrong, that is a lot of changes. Although, I do appreciate your good work. TeX ecosystem is incredibly large and I was partially intimidated from working on TeX support myself. |
|
I've done a rebase but the new-file-templates.nvim plugin is going to be an issue again for generating new tex files. Can you give me a rundown of how one might handle changing the source to the fork I made fixing the issue until the fix gets merged into the main repo? In the meantime, I'll start working on the fixes you requested. Just as a note, after rebasing by config became broken. The error was a big mess of nvim pluggin names from the looks of it (I can post the stacktrace error somewhere if you'd like but I don't know if here is where you want it). Removing |
|
LSPSaga is addressed in #629, alongside many other plugins. I'll merge it somehow tomorrow if my config decides to build.
|
…g newline characters to support tex templates properly
ab57610 to
4d80eeb
Compare
|
sorry for the mess, due to the diff being completely messed up and hard to read, I rebased your PR onto main |
horriblename
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall these are great additions, thanks for working on this!
There are a few architectural/code-style issues that needs fixing though. I should've stepped in much earlier, but I didn't expect the PR to be this big so I didn't look at it during the language module freeze... that was my bad
modules/plugins/formatter/conform-nvim/builders/builderTemplate.nix
Outdated
Show resolved
Hide resolved
modules/plugins/languages/tex/build/builders/builderTemplate.nix
Outdated
Show resolved
Hide resolved
modules/plugins/languages/tex/pdfViewer/getEnabledPdfViewer.nix
Outdated
Show resolved
Hide resolved
Thank you for the review. I have applied the changes you have requested to how I think you expect them to be. A second review of the updated changes would be much appreciated. |
Co-authored-by: Ching Pei Yang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things.
1. CI is failing because the formatting in the release notes is wrong. this comes from another PR, actually
That's a simple fix it can be ignored here, then
- this module is pretty different in terms of structure from other modules.
That's perfectly fine, but some documentation for eventual future maintainers could be useful. That can come in the shape of a multiline comment under [...]/build/default.nix, imo, or (if the other maintainers agree) with a separate page in the docs. This is just my 2 cents and I've not discussed it previously with the other maintainers, so it might become moot, but I think it might be worth thinking about.
The contents of this should more so explain why the decisions taken in the design of the module were taken, rather than how it's written or how it works. It's not a matter of it being "bad" right now (this is amazing work). More so, I want to avoid the unpleasant situation when, in the future, the original author's gone, or busy doing something else, and nobody remembers why the module holds this structure and we lose a bunch of time rediscovering it.
Again, the only real blocker for me is the CI failure. This second point is more of a thought and proposal. I know we don't normally do or ask this, but this is a really big module, so it might make sense
|
if you rebase on the current main, the CI should pass since #1301 addressed the issue |
| // (optionalAttrs cfg.build.enable { | ||
| build = { | ||
| inherit | ||
| (cfg.build) | ||
| onSave | ||
| useFileList | ||
| auxDirectory | ||
| logDirectory | ||
| pdfDirectory | ||
| filename | ||
| forwardSearchAfter | ||
| ; | ||
| inherit (builderCfg) args; | ||
| executable = with builderCfg; "${package}/bin/${executable}"; | ||
| }; | ||
| }) | ||
| # | ||
| # -- Forward Search -- | ||
| // (optionalAttrs (cfg.pdfViewer.enable) { | ||
| forwardSearch = { | ||
| inherit (pdfViewer) args; | ||
| executable = with pdfViewer; "${package}/bin/${executable}"; | ||
| }; | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how the builder and pdfViewer modules can be helpful, but it goes against what we had in mind for the LSP rewrite in v0.8; we wanted the vim.language.*.lsp to be simple, convenient toggles for common LSP configurations, and defer all user customization to the vim.lsp.servers option. The builder and pdfViewer modules go against this idea.
I think something that would be more in line with our design, is to simply include these as examples in the docs. I don't yet have an idea on how to do this well though.
If you have an idea on how to present these options in line with our design, you can try it, but I think we should just remove build and pdfViewer modules and revisit the issue in a future PR
Added support for the tex language.
Added the texlab LSP along with config options for build/compiling:
Not implemented (yet):
extraLuaSettingsoption for texlab that allows setting these options in the meantime):Defaults mostly follow the texlab defaults with some exceptions such as the default compiler being tectonic and the default file viewer being okular.
(I also alphabetically sorted the language imports in the modules/plugins/languages/default.nix)