Add options to customize citar-add-file-to-library

[mpedramfar]

PR for issue #804

[bdarcus]

Thanks!

The CI has flagged a few things you'll want to fix; seems mostly documentation related.

Also, note: we use conventional commit guidelines. If you want, you can rewrite to match them, or I can squash merge when done.

Related: you can just write "Close #840" and github will close the issue when the PR is merged.

[mpedramfar]

Thanks for the tips! It's probably easiest if you squash merge them when done.

[bdarcus]

This is really weird, and I guess a bug in GH. But @roshanshariff left a comment that is not now showing in the web UI.

I'll reproduce it below just to be sure:

Instead of checking citar-add-file-sources here, add a clause to the citar--check-configuration function and just call (citar--check-configuration 'citar-add-file-source). We'd like to centralize all such validation in one place.

If it shows up, I'll delete this.

[roshanshariff]

@bdarcus Sorry, I was doing a code review and accidentally submitted a single comment instead of submitting as a batch.

I like the general direction of this change. I just thought the UI for selecting a file source could be simplified using the read-multiple-choice function instead of reimplementing similar functionality. For example, suppose citar-add-file-sources was defined as follows:

(setq citar-add-file-sources
  '((?b "buffer" "current buffer" citar-add-file--from-buffer)
    (?f "file" "existing file" citar-add-file--from-file)
    (?u "url" "download from URL" citar-add-file--from-url)))

Then the code to select a file source would just be

(nth 3 (read-multiple-choice "Add file from" citar-add-file-sources))

This handles highlighting the shortcut character, shows extra help if requested, etc.

[mpedramfar]

I added the check for citar-save-file-function to citar--check-configuration. We can add the check for citar-add-file-sources after we're certain about its type.

When it comes to citar--check-configuration, there are some things in the code seem inconsistent.

1. error vs user-error: Currently citar--check-configuration checks if the type of citar-library-paths is correct (a list of strings) but it doesn't check if it's non-nil. If the type is incorrect, it raises an error, with backtrace and everythin. Inside citar-add-file-to-library it's checked if this variable is non-nil and if it is nil, it raises a user-error.

It's not clear to me why error is being used at all. Wouldn't user-error be better? And wouldn't it be better if we check if citar-library-paths is non-nil inside citar--check-configuration?

2. There seems to be a lot of customization options that do not appear in citar--check-configuration. Here are the the source/resource variables:
   • citar-open-resources: not checked
   • citar-file-sources: not checked
   • citar-notes-sources: not checked in citar--check-configuration. Instead, there is a separate function that checks a single source (i.e. citar--check-notes-source) and this is only called from citar-register-notes-source. If the user changes citar-notes-sources using setq or the customize interface, it won't be checked at all.

Wouldn't it be better to move all the checks to citar--check-configuration?

[roshanshariff]

Another small suggestion: instead of having the file sources return a (function . plist), just have them return a plist with the function included under a :write-file key. In the future, we could have other kinds of file sources that don't fit that abstraction (e.g. open and return a new Emacs buffer) and expose actions other than :write-file.

[roshanshariff]

@mpedramfar, yeah the usage of citar--check-configuration is somewhat inconsistent at the moment. Ideally, it would check the types of customization variables and raise user-error. Any functions that have additional requirements (like citar-library-paths being non-empty) would do that check on their own, after the call to citar--check-configuration.

New code should follow this pattern, and over time the old code can be changed as well.

EDIT: The idea is that citar--check-configuration could eventually be replaced (as far as possible) with an automated function that checks its value against the defcustom :type. Any functions that have additional preconditions will need to do their own checking.

[mpedramfar]

I see, there is a use of citar-library-paths that doesn't need it being non-nil, so it makes sense to check it only when we need it to be non-nil. There is no use case of citar-add-file-sources when it is nil. Is there a way to specify this in defcustom :type? Should we check for citar-add-file-sources being non-nil in citar--check-configuration or citar-add-file-to-library?

[roshanshariff]

Now that I look at it, it doesn't seem like there's a good built-in type for non-nil lists. I think it's fine to go back to what you had, just checking the list is non-nil before using it.

[bdarcus]

So what's the status here, given the back-and-forth?

[mpedramfar]

I applied the comments. Let me know if there are other issues to fix.

[roshanshariff]

I pushed a couple of commits with some changes. I also tested the code a little, but @bdarcus if you have time to give it a whirl, that couldn't hurt :)

If we don't discover any issues, I'm okay with merging now!

• I copy-edited the docstrings and refined the type declarations to present a better customization UI.
• @mpedramfar, lambda functions don't need to be quoted because they are self-quoting forms. And since we're using lexical bindings, lambda functions can capture local variables from the surrounding context. I made the needed changes.
• The "add file from buffer" function was using write-file. I changed it to use write-region instead. The former makes the buffer visit the destination file and marks it as unmodified, which struck me as rather dangerous. Instead, we should probably just write the contents of the buffer to the destination file but not touch the buffer in any other way, which is what write-region does. What do you think? EDIT: Another option would be to choose depending on whether the buffer is already visiting a file; if it isn't we'd do a write-file which would act like save-buffer. Otherwise we would write-region which wouldn't change the buffer at all.

Thanks @mpedramfar for suggesting this change! I'm looking forward to writing more functions to do things like saving to Zotero libraries (which is what I use). Then we can probably rename citar-add-file-to-library to just citar-add-file. When I have time, I'd like to collect some of the integration code that's floating around into a citar-zotero package.

[mpedramfar]

Thanks for the tips!

Using write-region makes sense. I think the only exception is the one you mentioned, when the buffer is visiting the destfile that we want to save into. In that case, calling write-region saves the buffer to its file, but calling buffer-modified-p returns t. So it makes sense to simply call save-buffer instead. I implemented that in the last commit.

As a side-note, you might find the Zotra package useful. It basically gives Emacs one of the main features of Zotero, i.e. capturing bib entries and attachments from the web. After this PR is merged, I'll add the integration code to Zotra, so that it can be added as one of the sources in citar-add-file-sources.

[bdarcus]

CI is complaining :-)

[mpedramfar]

Should be ok now :)

[bdarcus]

Merged; thanks!

I did test the basics, and didn't see any issues. But I don't tend to use this command anyway!