search

ali-ramadhan / DocumenterCitations.jl

DocumenterCitations.jl uses Bibliography.jl to add support for BibTeX citations and references in documentation pages generated by Documenter.jl.
https://ali-ramadhan.github.io/DocumenterCitations.jl/dev
MIT License
65 stars 10 forks source link

Fix for KeyError in expand_citations #56

Closed fingolfin closed 2 years ago

fingolfin commented 2 years ago

Fixes #55 for me.

fingolfin commented 2 years ago

@simonbyrne @LazyScholar does this look sensible?

fingolfin commented 2 years ago

It sure would be good, and if there were existing tests, I'd have tried to add one foe this patch. Alas, there are no real tests other than for some low level helpers :-(.

I think a good way to add tests to this package (which are lacking in general) would be to build an actual manual (eg that of the package itself) as part of the tests, and then compare the generated files to some reference versions (to detect changes; one may want to use a pinned version of Documenter for that... and/or specifically test against different Documenter versions to make sure it keeps working with both old and new versions...).

HOWEVER I do not have the time to write such a setup, sorry. If someone else does I could try to figure out how to add a test for this specific issue.

Of course other tests could be done that test parts of the code more specifically but I wouldn't know where to start. And how to so it w/o relying on undocumented parts of Documenter (not saying it isn't possible just that I lack the expertise).

BTW tracking code coverage for the test suite also would be nice.

fingolfin commented 2 years ago

I've added a simple test now, after I realized that even doctest(DocumenterCitations; fix=true) fails right now. PR #57 adds that test (but of course has failing CI) while this PR passes CI.

charleskawczynski commented 2 years ago

I guess, regardless of the tests, these changes look reasonable to me, so I'm fine with these changes overall. But it'd be nice to get feedback from others

simonbyrne commented 2 years ago

Thanks!