Skip to content
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

Make bib parser configurable #683

Closed
wants to merge 1 commit into from
Closed

Make bib parser configurable #683

wants to merge 1 commit into from

Conversation

bdarcus
Copy link
Contributor

@bdarcus bdarcus commented Aug 29, 2022

Implement #680.

This should be enough to enable #397?

Please review and comment.


On the org-bibtex parser, etc

I don't think I have time to do the org-bibtex parser and updatep functions. Would be great if someone can test this with that.

The first should look something like the below, and return a hash-table exactly like parsebib-parse does:

(cl-defun citar-cache--org-bibtex-parser (files &optional &key entries fields)
  "Return a parsebib-compatible hash-table for a list of org-bibtex FILES."
  ;; FILES is a list of file paths, though in citar, will only be a single item list
  (map-into                                ; convert alist into hash-table
    (org-element-cache-map ...

EDIT - I tried to figure out how to add the parser to parsebib on this branch:

https://github.com/bdarcus/parsebib/tree/org-bibtex

But I ran into a UX and technical issue: to take advantage of org-element-cache-map, you can't use temp-buffers, and need to be in the org major-mode. So unlike the other formats, it would seem to be more sensible to open all listed org files. But that's a departure from how parsebib and citar work. And what happens if someone does something crazy like do file-per-reference using org?

This may suggest that parser is better off here, and we just clearly document the different workflows.

citar-cache.el Outdated Show resolved Hide resolved
@bdarcus bdarcus marked this pull request as ready for review August 29, 2022 18:46
@bdarcus bdarcus requested a review from roshanshariff August 29, 2022 18:46
@bdarcus bdarcus marked this pull request as draft August 29, 2022 18:55
@bdarcus bdarcus marked this pull request as ready for review August 29, 2022 19:01
@bdarcus bdarcus force-pushed the conf-parse branch 4 times, most recently from 19d4df2 to d65761a Compare August 29, 2022 23:25
citar-cache.el Outdated Show resolved Hide resolved
citar-cache.el Show resolved Hide resolved
@bdarcus
Copy link
Contributor Author

bdarcus commented Aug 30, 2022

@andras-simonyi - when you have time, can you review this simple PR?


(defconst citar-cache--parser
(list
(cons t (cons #'parsebib-parse #'citar-cache--update-default-bibliography-p)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is maybe a bit ugly; does that matter?

@aikrahguzar
Copy link
Contributor

I am not sure how much this will help. The configuration it introduces is seems unlikely to be used outside of citar since a new parser is likely a lot of work. If the intention is to allow formats not supported by parsebib think a simpler way would be to essentially define an analog of parsebib-parse that does the calls to parsebib-parse-bib-buffer etc as parsebib-parse and includes the additional calls too. If a user really wants to support some format outside of citar, this function can be advised.

@bdarcus
Copy link
Contributor Author

bdarcus commented Sep 3, 2022

That's a good point.

I went this way because I wasn't sure if we want the org-bibtex parser in the citar package, or in a separate package. I'm still not sure on that actually, largely because there are outstanding UX and performance issues to sort out there.

WDYT?

@aikrahguzar
Copy link
Contributor

I went this way because I wasn't sure if we want the org-bibtex parser in the citar package, or in a separate package. I'm still not sure on that actually, largely because there are outstanding UX and performance issues to sort out there.

WDYT?

So the problematic scenario is when the parser is in a separate package and we don't want to depend on it? I think the soft dependency can be handled via featurep check inside the citar version of parsebib-parse and in that case we can warn users that they need the package installed if we are called to parse an org-bibtex file.

@bdarcus
Copy link
Contributor Author

bdarcus commented Sep 26, 2022

@aikrahguzar

So the problematic scenario is when the parser is in a separate package and we don't want to depend on it?

That's the immediate issue I was thinking of. But perhaps there could be others in the future?

@bdarcus bdarcus closed this Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants