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

Add prefix argument to force reloading of the cache #122

Merged
merged 6 commits into from
May 2, 2021

Conversation

publicimageltd
Copy link
Contributor

For all interactive functions, pass the prefix to `bibtex-read' to
allow optionally enforcing the reloading of the bibliography cache.

This is to avoid having to call bibtex-actions-refresh manually if the library file has been changed.

For all interactive functions, pass the prefix to `bibtex-read' to
allow optionally enforcing the reloading of the bibliography cache.
@bdarcus
Copy link
Contributor

bdarcus commented May 1, 2021

Thanks for this!

A couple of questions:

  1. do you know what happened with all the formatting/whitespace changes?
  2. can you explain how this works from a user POV?

I will also mention that I had in mind optimization an external notify-based auto cache refresh, as hinted at in the wiki, preferably contributed by someone with more experience with that than I. I did elect, however, NOT to include bibtex-completion-init-like support for this out-of-box.

But that doesn't mean we can't do both of course.

@publicimageltd
Copy link
Contributor Author

1. do you know what happened with all the formatting/whitespace changes?

oops, my elisp mode has a hook which calls `whitespace-cleanup' for every save.

2. can you explain how this works from a user POV?

The idea is that you can trigger reloading before doing anything with your bibtex stuff. So I export my library by going to Zotero, export the library manually, go back to emacs, and then press C-u .

I often work with the browser to add references to Zotero. Then I want to store a note, go to Emacs, and find out that the library has not been refreshed. I sigh, force refresh manually by calling bibtex-actions-refresh, and then repeat my action. So that's supposed to be shorter now: If I notice that the content is not up to date, I repeat the command with prefix.

I will also mention that I had in mind optimization an external notify-based auto cache refresh

bibtex-completion uses this approach, and I do not like it. It blocks Emacs too long and is sort of unpredictable. Plus, it means that I still have to remember to export the library manually.

Actually the PR isstep one for another solution I have in mind. The idea to recreate the .bib file from within Emacs, so that I can just leave Zotero untouched. I am right now in the process of writing a small library for that, which basically wraps a curl call to Zotero to update the .bib file. With this function, I would then like to add an option to bibtex-actions-refresh which automatically recreates the bib file (from within Emacs) before recreating the cache (something like a hook).

This solution, once implemented, would allow the user to bypass any interaction with Zotero. You launch Emacs, you launch Zotero, you put Zotero in the background. You import stuff in the browser, then go to Emacs to add comments etc. You call a bibtex-action with C-u, and everything is fine.

@bdarcus
Copy link
Contributor

bdarcus commented May 1, 2021

1. do you know what happened with all the formatting/whitespace changes?

oops, my elisp mode has a hook which calls `whitespace-cleanup' for every save.

I'm not super familiar with the tooling landscape around this with elisp.

So you're using a built-in command for that; not this package?

https://github.com/purcell/whitespace-cleanup-mode

I feel like we need some standardized solution, to avoid the repo history getting loaded up with diffs of whitespace.

Any thoughts on that?

For example, in another project I work on, we have a CI check with prettier for the js and markdown content, so I can basically say to devs, make sure to run prettier on your code, or it will get flagged.

How to deal with that for elisp?

2. can you explain how this works from a user POV?

The idea is that you can trigger reloading before doing anything with your bibtex stuff. So I export my library by going to Zotero, export the library manually, go back to emacs, and then press C-u .

I often work with the browser to add references to Zotero. Then I want to store a note, go to Emacs, and find out that the library has not been refreshed. I sigh, force refresh manually by calling bibtex-actions-refresh, and then repeat my action. So that's supposed to be shorter now: If I notice that the content is not up to date, I repeat the command with prefix.

So I'm still unsure how this works.

You do C-u and immediately M-x?

Ah, I guess so: https://www.emacswiki.org/emacs/PrefixArgument.

I will also mention that I had in mind optimization an external notify-based auto cache refresh

bibtex-completion uses this approach, and I do not like it. It blocks Emacs too long and is sort of unpredictable. Plus, it means that I still have to remember to export the library manually.

Actually the PR isstep one for another solution I have in mind. The idea to recreate the .bib file from within Emacs, so that I can just leave Zotero untouched. I am right now in the process of writing a small library for that, which basically wraps a curl call to Zotero to update the .bib file. With this function, I would then like to add an option to bibtex-actions-refresh which automatically recreates the bib file (from within Emacs) before recreating the cache (something like a hook).

This solution, once implemented, would allow the user to bypass any interaction with Zotero. You launch Emacs, you launch Zotero, you put Zotero in the background. You import stuff in the browser, then go to Emacs to add comments etc. You call a bibtex-action with C-u, and everything is fine.

Cool!

@publicimageltd
Copy link
Contributor Author

Thanks for the hint to whitespace-cleanup-mode, that sounds like a better solution. It actually calls the built-in function, but only if some criteria match. And yes, it would be good to have a standard there. But I don't know what's used by other projects...

@publicimageltd
Copy link
Contributor Author

Here's the link to the tool for force-exporting the library: https://github.com/publicimageltd/pullbib
If you approve this commit, I would do another PR with an added hook bibtex-actions-before-refresh-hook, which then could be used to call pullbib-pull.

@bdarcus
Copy link
Contributor

bdarcus commented May 1, 2021

Thanks for the hint to whitespace-cleanup-mode, that sounds like a better solution. It actually calls the built-in function, but only if some criteria match. And yes, it would be good to have a standard there. But I don't know what's used by other projects...

This seems potentially promising:

https://gitlab.com/ideasman42/emacs-elisp-autofmt

I'm checking to see how the listed projects use it.

Edit: as I do, a number of those projects are by the same author. Also, not on MELPA.

Just installed lispy, and then quickly uninstalled it when it installed packages I didn't want.

If you approve this commit, I would do another PR with an added hook bibtex-actions-before-refresh-hook, which then could be used to call pullbib-pull.

I'll approve it; just want to figure out the formatting issue.

Also, need to add a note about it to the README.

@bdarcus
Copy link
Contributor

bdarcus commented May 1, 2021

While I work on the formatting issues, I just ran the CI workflow (I have to approve it for first time PRs because I had a malicious PR awhile back), and it has a warning about one of the docstrings. Check the "files" tab and you'll see it.

Can you fix that please?

PS - I just pulled down the PR, and your formatting code is replacing spaces with tabs.

@bdarcus
Copy link
Contributor

bdarcus commented May 1, 2021

I've spent a bit of time reviewing the options for auto-formatting, and none seem ideal, which appears to be a consequence of lisp itself (it has a very flexible syntax).

It seems the most practical thing I can do is say to follow the Elisp style guide, which does say no hard tabs, etc., in the contributing doc, and maybe include a checklist to remind folks.

I strongly suggest you deactivate that auto-save solution you have, certainly when working on PRs for other projects which don't use it, because it means every PR you submit will confuse the diffs and history.

But I found this very cool solution to getting git to ignore whitespace changes, and to fix a commit that has them.

If you check the "inv-cache-clean" branch, the file (and here's the diff if you're curious) has your changes without the whitespace changes. I also fixed the docstring warning.

So you could simply copy that file over your local branch and commit it, and I can squash the commits on merge, or you could try the solution I used yourself:

https://stackoverflow.com/a/29986651

@publicimageltd
Copy link
Contributor Author

Sorry for the whitespace mess. Didn't even think that that function whitespace-cleanup introduces tabs, since elisp convention is not to use hard tabs, and I assumed a built in function would just do everything right, in particular for elisp. Hope file is now in the right condition to merge.

@bdarcus
Copy link
Contributor

bdarcus commented May 2, 2021

Formatting/diff now looks perfect!

I added a README change as well; merging now.

Thanks again!

@bdarcus bdarcus merged commit 506fdcf into emacs-citar:main May 2, 2021
@bdarcus
Copy link
Contributor

bdarcus commented May 2, 2021

If you approve this commit, I would do another PR with an added hook ...

On this, I was wondering where there might be room for hooks. This sounds like a good example!

@publicimageltd
Copy link
Contributor Author

Just a note on the whitespace mess: I found out that setting the buffer-local variable indent-tabs-mode to nil teaches whitespace-cleanup to convert all tabs to spaces.

(add-hook 'emacs-lisp-mode-hook (lambda () (setq indent-tabs-mode nil)))

So in conjunction with Purcell's whitespace-mode, that seems to be the best choice.

@bdarcus
Copy link
Contributor

bdarcus commented May 2, 2021

So in conjunction with Purcell's whitespace-mode, that seems to be the best choice.

Good deal!

I found out that setting the buffer-local variable indent-tabs-mode to nil teaches whitespace-cleanup to convert all tabs to spaces.

And presumably not to touch spaces?

@publicimageltd
Copy link
Contributor Author

"Touching" meaning exactly what? If you mean: Leaving them "as-is", yes. But of course, empty lines and trailing spaces are removed. The documentation of whitespace-cleanup is not the most readable one, but it seems to work.

@bdarcus
Copy link
Contributor

bdarcus commented May 2, 2021 via email

@bdarcus
Copy link
Contributor

bdarcus commented Jun 8, 2022

@publicimageltd - I have currently removed this in #628, for two reasons:

  1. that branch has code that should automatically handle reparsing the bib file(s) if they change
  2. we added this code awhile back, but since this PR, which adds optional comprehensive support via filenotify (though given the above, this may go away too).

Any counter-argument?

@publicimageltd
Copy link
Contributor Author

Thanks for notifying me. I regularly use the prefix argument to create a new bib file. So my main use is not simply parsing / caching, but rather to trigger Zotero from within Emacs. From an abstract point of view, this is use case completely unrelated to the citar functions and could be removed. But for quite natural reasons, I would rather keep it. I see it more like a hook: If you want something to be done before you access your bibliography (and if you don't want it done every time), call the Citar functions with a prefix. I like it.

In sum: You may remove it, since I can simply advise these functions to have the same functionality, but I don't see any reason to not leave it since it is not so much about the cache but rather about "doing something before". But it might well be the case that this is a rather special use case. On the other hand, I imagine a lot of people have this need. Once the data base has grown, auto-updating the corresponding bib tex file, in my view, is not an option anymore (too long, blocks Zotero regularly).

@bdarcus
Copy link
Contributor

bdarcus commented Jun 9, 2022

I see it more like a hook: If you want something to be done before you access your bibliography (and if you don't want it done every time), call the Citar functions with a prefix. I like it.

If it's "like a hook", would a hook be better here?

Part of the reason I was asking is also wondering about using prefix args for other UI purposes.

But I'm not super knowledgeable about either prefix args or hooks!

@publicimageltd
Copy link
Contributor Author

Hooks are usually run every time something happens or is done. Have a look at the standard hooks listed in the Emacs Lisp manual. The current prefix handling, however, only triggers these actions if the associated function is called interactively with a prefix. So no, a hook would not behave the same way.

If you feel the need to remove the prefix handling to free it for other purposes, please go ahead. I am afraid not many people are using it, even though I personally think it is super practical and that it is nearly mandatory if one has a huge data base maintained by Zotero. But then, judging from the github traffic, nobody is using pullbib. Seems to be my personal itch, then. And really, the library can be pulled manually, nothing is lost.

@bdarcus
Copy link
Contributor

bdarcus commented Jun 9, 2022

I'm not committed to removing (or rather not re-implementing it with the new core code); why I was asking the questions ;-)

I'll put this on the list of details to settle, and link here.

Thanks!

@publicimageltd
Copy link
Contributor Author

A second thought: If the cache then is updated via filenotify or in other automatic ways, won't that block Emacs in the case of huge databases? That's the only problem I could see if we are talking about keeping the same functionality. I would like to control the recreation of the cache if it can't be done in the background smoothly.

@bdarcus
Copy link
Contributor

bdarcus commented Jun 9, 2022

A second thought: If the cache then is updated via filenotify or in other automatic ways, won't that block Emacs in the case of huge databases? That's the only problem I could see if we are talking about keeping the same functionality. I would like to control the recreation of the cache if it can't be done in the background smoothly.

Yeah, it could. But @roshanshariff has in mind it may be possible to do this, or parts of it, asynchronously somehow.

I do think the branch code is a lot of faster, but my bib file is only ~1000 entries, and I've not figured out a good benchmark yet to test this objectively.

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.

None yet

2 participants