Fix dangling braces

[aikrahguzar]

Closes #876

Also a performance improvement in the exporter: we insert the bibliographies into a buffer once instead of once for each citekey.

[aikrahguzar]

I don't have any idea about the eldev related failing test 😬

[aikrahguzar]

I think the test failure is unrelated. The same test also fails for the current main branch: https://github.com/emacs-citar/citar/actions/runs/22580821553

[bdarcus]

I don't have any idea about the eldev related failing test 😬

It says long documentation lines?

[bdarcus]

I clicked the explain thing on the failing commits, and copilot gave a plausible explanation, which it seems to have summarized below the commits.

[aikrahguzar]

I don't have any idea about the eldev related failing test 😬

It says long documentation lines?

I fixed that in bfecaff, the test that is still failing on later commits is due to eldev and involves

[00:00.663]  Symbol’s function definition is void: nil

and I have no idea where that is coming from.

I clicked the explain thing on the failing commits, and copilot gave a plausible explanation, which it seems to have summarized below the commits.

I don't see any explain thing!

[bdarcus]

The root cause of the CI failure is the removal of the function citar--insert-bibtex, while references to it remain in the code. Specifically, in citar-export-local-bib-file, there is a call to (citar--insert-bibtex citekey), but only citar-insert-bibtex now exists.

Solution:

Update the implementation of citar-export-local-bib-file to use citar-insert-bibtex instead. Here is the corrected snippet:
elisp
;;;###autoload
(defun citar-export-local-bib-file ()
"Create a new bibliography file from citations in current buffer.

The file is titled "local-bib", given the same extension as
the first entry in citar-bibliography', and created in the default-directory'."
(interactive)
(let* ((citekeys (citar--major-mode-function 'list-keys #'ignore))
(bib-files (citar--bibliography-files))
(ext (file-name-extension (or (car-safe citar-bibliography)
citar-bibliography)))
(file (expand-file-name (format "local-bib.%s" ext))))
(with-temp-file file
(citar-insert-bibtex citekeys bib-files))))
Also, search the codebase for any remaining calls to citar--insert-bibtex and update them to use citar-insert-bibtex.
This change will resolve the "void-function nil" error and allow CI to pass

[bdarcus]

I don't see any explain thing

Just copy-pasted it's output. Sorry for the formatting.

[aikrahguzar]

No,

The root cause of the CI failure is the removal of the function citar--insert-bibtex, while references to it remain in the code. Specifically, in citar-export-local-bib-file, there is a call to (citar--insert-bibtex citekey), but only citar-insert-bibtex now exists.

Solution:

Update the implementation of citar-export-local-bib-file to use citar-insert-bibtex instead. Here is the corrected snippet: elisp ;;;###autoload (defun citar-export-local-bib-file () "Create a new bibliography file from citations in current buffer.

The file is titled "local-bib", given the same extension as the first entry in citar-bibliography', and created in the default-directory'." (interactive) (let* ((citekeys (citar--major-mode-function 'list-keys #'ignore)) (bib-files (citar--bibliography-files)) (ext (file-name-extension (or (car-safe citar-bibliography) citar-bibliography))) (file (expand-file-name (format "local-bib.%s" ext)))) (with-temp-file file (citar-insert-bibtex citekeys bib-files)))) Also, search the codebase for any remaining calls to citar--insert-bibtex and update them to use citar-insert-bibtex. This change will resolve the "void-function nil" error and allow CI to pass

There are no references to citar--insert-bibtex. This is the current definition in this pr,

(defun citar-export-local-bib-file ()
  "Create a new bibliography file from citations in current buffer.

The file is titled \"local-bib\", given the same extension as
the first entry in `citar-bibliography', and created in the
`default-directory'."
  (interactive)
  (let* ((citekeys (citar--major-mode-function 'list-keys #'ignore))
         (bib-files (citar--bibliography-files))
         (ext (file-name-extension (or (car-safe citar-bibliography)
                                       citar-bibliography)))
         (file (expand-file-name (format "local-bib.%s" ext))))
    (with-temp-file file
      (citar-insert-bibtex citekeys bib-files))))

As far as I can tell, the test failure is not related to the pr. I manually triggered the CI on current main and it shows this same error: https://github.com/emacs-citar/citar/actions/runs/22580821553

[roshanshariff]

LGTM!

[roshanshariff]

I wouldn't worry too much about tests failing on the snapshot version of Emacs. Those tests are meant as a heads-up of breaking changes in Emacs master we need to be aware of. But they do fail inexplicably sometimes, and that should only be treated as a blocker after Emacs master has been branched for the next release.

[aikrahguzar]

I think we are facing this issue in eldev: emacs-eldev/eldev#120

[havarddj]

This is a really great feature, I'm looking forward to it being merged! I have one request though: could you make the file name an optional argument to citar-export-local-bib-file, and fall back to the usual "local-bib.(ext)" if none is provided? Or alternatively trigger a completing-read if called interactively.

My use case is this: I have a global .bib file which contains thousands of .bib keys (exported automatically via Zotero), but when I'm writing papers with collaborators, it's convenient to have a smaller bib file local to that repository (so my collaborators can more easily edit it). But my collaborators usually expect the bib file to be named "references.bib".

[bdarcus]

I wouldn't worry too much about tests failing on the snapshot version of Emacs. Those tests are meant as a heads-up of breaking changes in Emacs master we need to be aware of. But they do fail inexplicably sometimes, and that should only be treated as a blocker after Emacs master has been branched for the next release.

Maybe we shouldn't run the snapshot with warnings as errors?

Either way, I agree it's not worth holding up merging.

Though consider the suggestion from @havarddj?

[aikrahguzar]

This is a really great feature, I'm looking forward to it being merged! I have one request though: could you make the file name an optional argument to citar-export-local-bib-file, and fall back to the usual "local-bib.(ext)" if none is provided? Or alternatively trigger a completing-read if called interactively.

This makes sense. Hardcoded name is not ideal. Optional arg is straightforward so we should add that and I think interactively we can prompt with a prefix arg.

Beyond that should the default be made customizable? I think that makes sense but I don't use this feature so not sure. But that seem to be the best fit with @havarddj's workflow.

[havarddj]

Hi, sorry for the delay. I think having a customizable variable would be very helpful for my use-case, perhaps something like citar-export-local-bib-file-name.

[aikrahguzar]

@havarddj I pushed a new commit, please try that.

[aikrahguzar]

@roshanshariff @bdarcus tests are now failing because Embark can't be installed since it has started requiring Emacs 29. What should we do about that?

[bdarcus]

@roshanshariff @bdarcus tests are now failing because Embark can't be installed since it has started requiring Emacs 29. What should we do about that?

Maybe we should follow Embark (and Vertico)?

The alternative is changing the CI.

[havarddj]

@havarddj I pushed a new commit, please try that.

Hey @aikrahguzar!

I couldn't quite get it working, it doesn't seem to export all the references in the document. The smallest working example I could make was to add a file test.tex in the test/ directory, with the following contents:

\documentclass[11pt]{amsart}

\usepackage[
   backend=bibtex,
   style=alphabetic,
   natbib=true,
   url=false, 
   doi=true,
   eprint=false,
   backref=true
   ]{biblatex}
\addbibresource{test.bib}

\begin{document}
This is citation. \cite{downes-74}
This is another citation. \cite{mann-68}

\printbibliography
\end{document}

This compiles correctly (at least on my machine).
Running M-x citar-export-local-bib-file then creates a file local-bib.bib, but it's empty. In a more complex project, it would sometimes only add a couple of the bib entries, but (usually) not all. So perhaps there's a problem with citar-insert-bibtex?

I loaded the local version of citar by cloning the PR and running package-install-from-buffer inside citar.el (and I've confirmed the version of citar-export-local-bib-file is correct, since it has file as an optional argument). I also ran M-: (format "%s" (citar--major-mode-function 'list-keys #'ignore)), which returns "(mann-68 downes-74)". Unrelatedly, it would be nice if this list was sorted alphabetically, so the bib keys in the local bibliography are sorted as well!

[aikrahguzar]

Hey @aikrahguzar!

I couldn't quite get it working, it doesn't seem to export all the references in the document. The smallest working example I could make was to add a file test.tex in the test/ directory, with the following contents:

\documentclass[11pt]{amsart}

\usepackage[
   backend=bibtex,
   style=alphabetic,
   natbib=true,
   url=false, 
   doi=true,
   eprint=false,
   backref=true
   ]{biblatex}
\addbibresource{test.bib}

\begin{document}
This is citation. \cite{downes-74}
This is another citation. \cite{mann-68}

\printbibliography
\end{document}

This compiles correctly (at least on my machine). Running M-x citar-export-local-bib-file then creates a file local-bib.bib, but it's empty. In a more complex project, it would sometimes only add a couple of the bib entries, but (usually) not all. So perhaps there's a problem with citar-insert-bibtex?

I can't reproduce this. I see both entries in the local-bib.bib. Make sure that you call M-x reftex-parse-all before the export. Otherwise, reftex wouldn't report the test.bib file.

Unrelatedly, it would be nice if this list was sorted alphabetically, so the bib keys in the local bibliography are sorted as well!

Sorting, makes sense.

[havarddj]

Thanks, running reftex-parse-all does fix it for me! Works like a charm now.

[havarddj]

Out of curiosity, is there a reason why citar-insert-bibtex uses bibtex-search-entry under the hood but citar-open-entry-in-file uses parsebib? I was a little confused why it wouldn't export entries with @online, but that seems to be because bibtex.el is strict about what it considers an entry, and why for the other function parsebib is used: https://github.com/joostkremers/parsebib/discussions/23.

[aikrahguzar]

I have now made the failure to find an entry loud: a message will be emitted mentioning missing the entry along with the bib files that were searched. Am empty list of bibliographies will be an error.

Sorting is also implemented now.

[havarddj]

Awesome, thank you very much!

[bigodel]

Hi, coming over from #885, where @aikrahguzar pointed to this PR due to some overlap.

For context: #885 adds a configurable name for the file produced by citar-export-local-bib-file via a new defcustom that accepts string, function or nil, plus a couple of helpers (citar-local-bibtex-base-name deriving a basename from the buffer's bibliography declaration, citar--local-bibliography-files extracting per-buffer local bib lookup), a function rename for clarity, and tests. Significant overlap with the configurability piece of this PR.

My suggestion is to consider splitting scope:

• Fix dangling braces #879: keep focused on the dangling-braces bug (its primary motivation, ready to land).
• feat: Customizable basename for citar-export-local-bib-file #885: track the configurable-local-export-bib work (defcustom shape, helpers, tests, naming).

The brace fix could merge cleanly without being held up on UX/API discussion, and the larger conversation moves to a smaller follow-up PR.

If you'd rather keep everything together, happy to close #885 and move the pieces here as suggested changes. See this comment with some changes from #885 I'd like to see retained.

[aikrahguzar]

@bigodel, how much work would it be to rebase #885 on top of this? I can merge this and then we can move to #885. The problem with this splitting this is that is also fixes the issues raised in #834 so it is really the title of the pr which is inaccurate. It has become a reworking of local bib export.

[bigodel]

@bigodel, how much work would it be to rebase #885 on top of this? I can merge this and then we can move to #885.

I'm happy to rebase #885 on top of this once it merges.

One small ask: if a release is being cut on the back of this PR, would you consider also folding in the suggested renames, citar-export-local-bib-file -> citar-export-local-bibtex-file and citar-exported-bib-file-name -> citar-exported-bibtex-base-name. The new name reflects the fact that the exported file will be in BibTeX format (regardless of the extension) and that the variable's file name will only be used for its base name, as the extension is derived.

If a release isn't imminent, no urgency, I'm happy carrying the rename in #885 once I rebase. Either way works.