Conversation
lervag
left a comment
There was a problem hiding this comment.
Thanks! In addition to my comment to the code, could you also add a test file for this under test/test-syntax/test-tabularx.{vim,tex}?
|
Also sorry for the late reply! |
Don't worry about it. I know how it is to be an open source developer :) I moved the files and added some tests. I know I'm new around here, but have you considered using something like vader for the tests? It would be really nice to run a single test at a time. As far as I can tell, you can't with this makefile. |
:)
Great; thanks!
I actually much prefer my way of testing. It is really quite easy to run a single test at a time: cd $VIMTEX/test
# Run ALL tests
make
# Run all test-syntax tests
make -C test-syntax
# Run all test-syntax tests (alternative)
cd test-syntax
make
# Run only the tabularx test
make test-tabularx
# Run the tabularx tests but stay within neovim/Vim
nvim --clean -u test-tabularx.vim
vim -u test-tabularx.vim |
test/test-syntax/test-tabularx.tex
Outdated
| 1 & 2 & 3 & 4 \\ | ||
| \end{tabularx} | ||
|
|
||
| % Tabularx does not seem to have options |
There was a problem hiding this comment.
What do you mean with this? In the implementation, you do allow options..?
There was a problem hiding this comment.
I added texTabularxOpt for consistency with normal tabular, but as far as I can tell reading the tabularx docs, there are no supported [] options for this environment. Should we keep it like this for consistency and in case they decide to add some, or should we take it away?
There was a problem hiding this comment.
I think we should aim to implement the correct behaviour. If you are right, then we should not include the option group. I don't think you are wrong, but if so, it is easy to adjust later if someone shows a relevant proof.
|
I think the only thing lacking now is to set the default highlight groups. I think you can use this as an inspiration: https://github.com/lervag/vimtex/blob/master/autoload/vimtex/syntax/p/array.vim#L56-L66 |
|
I added some default highlight groups. How does that look? |
| \}) | ||
|
|
||
| highlight def link texTabularxArg texOpt | ||
| highlight def link texTabularxWidth texOpt |
There was a problem hiding this comment.
Perhaps it would be better to link this to texLength? If it is a number, at least?
There was a problem hiding this comment.
I might be getting the vocabulary wrong, but I think it's a length, not a number. For example, it's quite common to use \tabularx{\linewidth}....
There was a problem hiding this comment.
Yes, I believe it is a length. The texLength group matches things like 1em and similar, and I believe it is a better default highlight group.
|
Looks more or less good! I have one comment, and also a response regarding the option group. I looked at the docs, and I notice the syntax description looks like this: \begin{tabularx}{WIDTH}[POS]{PREAMBLE}Thus, there is a possible option group after the WIDTH argument. The WIDTH argument does not need a default highlight group, but it should contain the |
|
Sorry for the delay. I think I got what you mean. Also, thanks for explaining how to run only the tests for I just realise that the first link for |
No problem!
Ah, cool. Then we should keep the |
Sorry, I thought you meant to put the |
|
Thanks, I've merged now with a few minor adjustments. The most important one: The |
This properly highlights the arguments of the
tabularxenvironment, which takes two arguments unliketabularthat only takes one. The reason I added this was so that the second argument would properly get recognized as@texClusterTabular. Otherwise, the column specifiers get marked as a misspelled word when usingset spell. This is a big problem for me because I use]sand[sto jump around and fix things. It is also preferable to have proper highlight groups for more reasons than just spelling.