Conversation
lervag
left a comment
There was a problem hiding this comment.
Thanks. This does seem pretty useful. It would be nice to have a simple example to test this. Perhaps we could even make a test?
lervag
left a comment
There was a problem hiding this comment.
From #2037:
The test used to always take the second path because vimtex doesn't know about the biblatex package being loaded before a compile.
Ah, yes: there are now three branches in the #files() function: 1) the "biblatex + bcf" branch, 2) the "blg" branch, and 3) the manual branch. And the old code only checks the third.
However, I propose, instead of doing the compilation every time, we simply add a bcf file to the test assets. Then we can simplify everything quite a lot. I.e., we can have test_globbed.bcf with only a single line:
<bcf:datasource type="file" datatype="bibtex" glob="false">test_gl*.bib</bcf:datasource>Of course, this will force the first branch to execute, and then the third one is not tested. So, to test both the old and new branches, it might be necessary, or at least simpler, to make two test files.
The following seems to work quite well:
set nocompatible
let &rtp = '../..,' . &rtp
filetype plugin on
nnoremap q :qall!<cr>
let g:vimtex_cache_root = '.'
let g:vimtex_cache_persistent = 0
silent edit test_globbed_manual.tex
if empty($INMAKE) | finish | endif
let s:candidates = vimtex#test#completion('\cite{', '')
call vimtex#test#assert_equal(1, len(s:candidates))
bwipeout!
silent edit test_globbed_bcf.tex
let s:candidates = vimtex#test#completion('\cite{', '')
call vimtex#test#assert_equal(2, len(s:candidates))
quit!% test_globbed_manual.tex
\documentclass{article}
\addbibresource{test_gl*manual.bib}
\begin{document}
Test
\end{document}% test_globbed_bcf.tex
\documentclass{article}
\usepackage{biblatex}
\addbibresource{test_gl*bcf.bib}
\begin{document}
Test
\end{document}And where the *manual.bib has one key and *bcf.bib has two keys. What do you think?
8540bea to
1e7c67c
Compare
|
This now relies on vimtex not throwing out duplicate bibliography keys. If this ever changes, one would need to replace the symlinks with proper bib files. Copies of the original with slightly different citation keys should be enough. Of course that would open the possibility for your idea of different numbers of keys per file, maybe powers of two so one can easily see from the assertion failure message which files weren't found. |
|
Thanks! FYI, this does not pass tests for me. In particular, the manual route does not pass, and it seems to be something with the caches. Perhaps there's a bug in the cache mechanism, but I'm curious, why is not my much simpler version of the tests sufficient here? |
This seems to be a flakey test. Before I pushed, the test was failing for me on top of v2.4 and passing on top of this here branch, as it should. On repeat tries I can also observe failures, but not conistently. That is very strange, but I also don't know anything about vimtex's cache architecture to know if that could be the reason. I've tried inserting
You mean the two tex files with their own bibs? I wanted to reduce complexity while also testing more glob cases. But if my approach confuses the cache mechanism, then I should just implement your proposal to get the test to work reliably. |
|
I'm not fully sure if the problem is caused by the caches, actually, but it seems like a probable cause. Still, when I looked into it, I could not find a consistent way to reproduce or avoid the issue.
Yes, exactly. I understand the idea of reducing complexity and allowing more tests in a more automatic fashion. However, please alos note that the way you implement the tests is much harder to "reason" about. My suggestion may be more explicit and verbose, but it is also mostly trivial to reason about and understand. In my opinion, this is a good thing. It should also be quite easy to add more similar tests based on the same simple mechanism. PS! This is getting into a long thread on testing technicalities where your suggested patch is clearly an improvement. I don't want to "scare" you from contributing by being pedantic. So, I just want to be clear that I already much appreciate your contribution and I won't mind to just write the test myself and merge this. |
1e7c67c to
1e0ff29
Compare
|
I see you've credited me in the release notes for 2.5, yet I can't see any changes to I've reworked the test cases to be clearer and fixed another problem in the process. Hopefully, we can get this problem fixed now. :) |
The fix in 2efe806 isn't used if biblatex is detected, even though globbing is only possible with biblatex. Since b7fbff1, that code path is only used if there aren't better methods of detecting bib files. That was okay because the only other method introduced was to parse BibTeX's blg file and BibTeX can't handle globbing anyway. But in 2536f04, support for biber's bcf file was added without also adding globbing support. Apparently, globbing can be disabled per resource, so filenames that are candidates for globbing must be retained in the file list. This should only have an effect with curly braces, though. Globbing candidates that aren't actual filenames will be filtered out with s:validate(), as before.
1e0ff29 to
a558e91
Compare
|
Sorry for the repeat force-pushes, but I wanted to have all the tests for completions that should have worked in one commit. |
Huh, strange. That probably a mistake, perhaps because I noticed #2037 that was closed. Sorry about that; I hope you don't mind that I edit the release notes. You will then be properly credited in the next release notes.
Great! I think the tests look good, except for some reason, they are failing. Do you immediately notice why?
No problem; I prefer logical commits anyway :) Feel free to also rebase your PR on top of the most recent master. Of course, I'll make sure to do it anyway before I merge, but in the small chance of a merge conflict it may help to have your eyes on the conflict matter.
Yes, I think they work similarly. |
|
Tests fails with following message: |
|
It seems the problem is here: When I print the value of |
|
That must be vim-specific; I have to admit that I've been testing with neovim only, which doesn't throw an error for invalid globs. I'm not a fan of pre-checking potential globs for validity as that's already being done in |
Nope. It fails also on neovim (see the Github actions result here).
Perhaps; if that would work. I don't mind checking more candidates, but we need to be sure the output is validated properly. |
|
Which version of neovim are you on? |
|
|
Can you try the latest release (v0.5.0)? |
Fedora 34 doesn't yet carry that version and I'm not inclined to compile it myself, especially when the version used in the failed Github Action is the same as mine and it's therefore questionable whether the version itself is the problem. For some reason, my neovim is unable to complete another test, even with Dunno why that is, maybe I've just got an entirely broken installation. Maybe I should just do my tests in a container? |
You should not need to set the The tests pass on master both on my systems and on Github Actions, so this does seems somewhat strange. :\ |
|
I've think I've found a reasonable solution. Let me know what you think (see my extra commit). |
|
Yeah, that'll do. I didn't know that vim could do error catching. A bit disappointing that my PR couldn't be merged but very glad that the problem's been fixed to your satisfaction now! |
|
Oh sorry, I'm just now seeing that my commits have, in fact, been merged. No reason for me to be disappointed, then. :) |
Yes, as I often say, Vimscript is not as bad as many people think. But it is peculiar at times. ;)
Yes, sorry if that was unclear. I rebased your commits on top of master, then added a minor commit where I made some changes that I believe were minor improvements. I try to keep the contributions from others clear and visible; I very much appreciate when others want to help and contribute! And thanks for the kind words! |
|
By the way; the tests still fail. They pass on my system, so it must be a version or system related issue. Do you have any clue what might be wrong? https://github.com/lervag/vimtex/runs/3042079001?check_suite_focus=true |
The fix in 2efe806 isn't used if BibLaTeX is detected, even though globbing is only possible with BibLaTeX's biber.
Since b7fbff1, that code path is only used if there aren't better methods of detecting bib files. That was okay because the only other method introduced was to parse BibTeX's blg file and BibTeX can't handle globbing anyway. But in 2536f04, support for biber's bcf file was added without also adding globbing support.
Apparently, globbing can be disabled per resource, so filenames that are candidates for globbing must be retained in the file list. This should only have an effect with curly braces, though. Globbing candidates that aren't actual filenames will be filtered out with
s:validate(), as before.See also: #1658