pre-1.0-cleanup branch

[bdarcus]

I was realizing I was making too many small commits, so setup this branch to collect various commits to get this repo ready for tagging 1.0 in the next week or so.

The first commit emerged out of a suggestion from the MELPA review for citar-embark, and that is to use these for the file headers:

https://spdx.dev/ids/

So I've done that, and also removed the human-readable license text. I hope that's OK; I guess it should be if MELPA folks recommended it.

@localauthor and @aikrahguzar - not sure if you wanted your names on the copyright for some of these files?

---

Questions/Issues

1. I'm not sure on this, but on the note API, I'm leaning towards only defining the :action function (or we could all it :open?), and leaving out the :create one. My reasoning is that creating new notes is pretty closely-tied to opening them, and so better to just have the API reflect that. Yes, if someone wants something like the default source for markdown, it will mean having to copy and adjust the default function(s). But that seems a small price to pay. And at least with citar-org-roam, there's no value in a separate configurable function for new notes.

[aikrahguzar]

@localauthor and @aikrahguzar - not sure if you wanted your names on the copyright for some of these files?

I don't think my name is needed on on the copyright.

I haven't tested the new interface yet but I will over this weekend.

[localauthor]

@localauthor and @aikrahguzar - not sure if you wanted your names on the copyright for some of these files?

I don't think my name is needed on on the copyright.

Agreed. No need to worry about it for me. :)

I'm testing and reconfiguring some of my custom functions now. All seems good so far, and its easy to see the API is much more sensible/consistent. I'll be sure to flag any issues when/if I come across them.
Thanks for the work, and for checking about this!

[bdarcus]

@localauthor - OK cool.

Do you use Embark, such that you're testing the new minor mode?

[localauthor]

I am testing the Embark minor mode, yes, and all seems ok so far.

[bdarcus]

And remind me: do you use markdown for notes, and your note package?

If yes, any thoughts on my question 1?

Might be we add a note source config for markdown, at least to the wiki? So I'm curious what that should look like, to answer that API question.

Perhaps that's what you're working on now?

[localauthor]

And remind me: do you use markdown for notes, and your note package?

If yes, any thoughts on my question 1?

Might be we add a note source config for markdown, at least to the wiki? So I'm curious what that should look like, to answer that API question.

Perhaps that's what you're working on now?

I primarily use org-mode, not markdown. But I've used some homecooked advices to access my notes through citar, so it'll take a bit of reworking before I can comment on the note source API specifically.

One thing: I think citar-open-notes should be added to citar-embark--multitarget-actions, right?

[localauthor]

Ok, so I've just made a note source tailored to my zk package, here: zk-citar

Re: question 1 above, I think :open makes more sense than :action. And no opinion on leaving or taking out :create, except that leaving it in makes things a bit more transparent. That said, for zk-citar, as with citar-org-roam, there is no value in a separate create function. Seems like a toss up.

[bdarcus]

And no opinion on leaving or taking out :create, except that leaving it in makes things a bit more transparent.

The one wrinkle here is the distinction between format and create.

With the former, we could use same open function, but different format functions, for markdown and org.

I lean, then, towards :open and :format, which I can easily adapt to both citar and to citar-org-roam; though in the latter case, technically that function creates the note file also.

Or could just write the docs in such a way it's clear it's up to the developer to do what they want with the functions?

[roshanshariff]

This is one place where the API is a little non-orthogonal. If your note-taking package is agnostic about Org/Markdown/etc., it still has to be able to format the note appropriately because that is the responsibility of the note source. Even though perhaps citar-org or citar-markdown are in a better position to handle that. I'm not sure if/how to improve this.

[bdarcus]

This is one place where the API is a little non-orthogonal. If your note-taking package is agnostic about Org/Markdown/etc., it still has to be able to format the note appropriately because that is the responsibility of the note source. Even though perhaps citar-org or citar-markdown are in a better position to handle that. I'm not sure if/how to improve this.

We should rename :action to :open regardless.

But beyond that, it seems the options are:

1. keep a single function, and leave it developers to determine how to handle creating new notes.
2. also have a single :create or :format property
3. add both, and clearly distinguish them

Right?

[roshanshariff]

I agree there should only be one function, just as emacs has find-file for both opening or creating files. The question is what that function should do after it has created a note:

1. Format the note as it sees fit. Perhaps citar-org-roam will do this, because the notes are necessarily in Org format.
2. Just call find-file and do nothing else. This will give a blank note by default.
3. Call find-file, then use some Citar function that we provide for formatting notes based on the major mode.

This is of course a question for the developer of the note package, but if they want Citar to handle the note formatting (option 3) then we might able to provide a function to help them.

[bdarcus]

This is of course a question for the developer of the note package, but if they want Citar to handle the note formatting (option 3) then we might able to provide a function to help them.

Does that not suggest an (optional) :format function?

So then the default (citar-file) would include that, and citar-org-roam and zk would probably not.

That reminds me; we should rename citar-file to citar-file-org or some such.

[roshanshariff]

Does that not suggest an (optional) :format function?

Would Citar call this after the :open function has created a note? In that case, I don't think we need it; the :open function could just call the formatting function itself, couldn't it? Instead of telling Citar what to run via the :format function?

That reminds me; we should rename citar-file to citar-file-org or some such.

The citar-file note source has very little to do with Org except for the formatting when it creates a new note. This suggests that the formatting should be handled by Citar rather than the note source.

The simplest way to do this would be to add a defcustom citar-format-note-function, which the note :open function calls when it has created new note. By default, its value could be citar-org-format-note-default, but the user can change it as they please.

Another option would be to add a command like citar-format-note, which is modeled on the emacs auto-insert function. It would look at the major mode of the current file, look up the ref to which the note belongs, and dispatch to a major-mode specific function that actually inserts the note format into the current buffer.

We'd need a new note API function (:noteref?) which returns the citekey for the currently open note (or nil if the current file isn't a note). This seems useful to add anyway, because we could use it to offer actions that operate on the current ref.

[bdarcus]

Does that not suggest an (optional) :format function?

Would Citar call this after the :open function has created a note? In that case, I don't think we need it; the :open function could just call the formatting function itself, couldn't it? Instead of telling Citar what to run via the :format function?

Yes. I was just doing the thought experiment of asking what it would look like to add a markdown function that otherwise works the same as the current org one.

But I do think perhaps if in doubt, we should stay with just the one.

That reminds me; we should rename citar-file to citar-file-org or some such.

The citar-file note source has very little to do with Org except for the formatting when it creates a new note. This suggests that the formatting should be handled by Citar rather than the note source.

Except, would that generalize?

What if someone wanted a note source where there was a single file, and markdown?

The simplest way to do this would be to add a defcustom citar-format-note-function, which the note :open function calls when it has created new note. By default, its value could be citar-org-format-note-default, but the user can change it as they please.

Another option would be to add a command like citar-format-note, which is modeled on the emacs auto-insert function. It would look at the major mode of the current file, look up the ref to which the note belongs, and dispatch to a major-mode specific function that actually inserts the note format into the current buffer.

We'd need a new note API function (:noteref?) which returns the citekey for the currently open note (or nil if the current file isn't a note). This seems useful to add anyway, because we could use it to offer actions that operate on the current ref.

Do you think it's something you're confident we need to add now, or should we just add it later?

For example, one wrinkle with this is org-roam allows more than one ref per node, while I suspect most systems wouldn't. But I guess we'd want to be more general here?

My impulse is the latter (hold for later, until we actually need it). I'm less concerned with adding note functions later, than changing how they operate.

[bdarcus]

For now, I just pushed 7a04d3e

[localauthor]

Lest this get lost in the shuffle:

Should citar-open-notes should be added to citar-embark--multitarget-actions?

[bdarcus]

Lest this get lost in the shuffle:

Should citar-open-notes should be added to citar-embark--multitarget-actions?

Sorry; meant to add that also today. Done.

[roshanshariff]

I've pushed a new commit that makes some API changes. As @bdarcus hoped in #634, the API functions no longer take an optional entries argument to prevent them from using the cache. Instead, there is a buffer-local citar--entries variable which can be let-bound when needed to achieve the same effect.

This change also affects notes sources since the :hasnote and :items functions no longer get the entries argument. Instead, you can just use citar-get-entry, citar-get-entries, or citar-get-value, etc. if needed.

I've also simplified some of the code around citar-open. See the commit message for details. As part of this, there is a new optional :transform function for notes sources which can be used to change how note items are displayed. (They're used in the grouping function.)

[bdarcus]

@roshanshariff - do you mean, for example, citar-org-roam?

If yes, the answer is yes; it works.

[bdarcus]

Correction: something is broken.

Unrelated to this, but with citar-open:

Debugger entered--Lisp error: (void-variable embark-default-action-overrides)
  citar-open(("ahrens2017"))
  funcall-interactively(citar-open ("ahrens2017"))
  command-execute(citar-open record)
  execute-extended-command(nil "citar-open" nil)
  funcall-interactively(execute-extended-command nil "citar-open" nil)
  command-execute(execute-extended-command)

... which I can't figure out, because you declare the variable.

I did push a tiny fix for a syntax error (let -> let*) in citar--library-file-action, that I squashed into your commit, but that doesn't resolve it.

BTW, I created a separate branch with the updated :open property for citar-org-roam, if you want to test it.

https://github.com/emacs-citar/citar-org-roam/tree/cit-1.0

EDIT: I earlier edited this noting a bug with citar-open-notes, but that turned out to be user error :-)

[roshanshariff]

I just tested it and citar-open doesn't actually work when opening notes with citar-org-roam:

1. Set citar-notes-source to 'citar-org-roam.
2. Run citar-open and select a ref with notes.
3. Select a note and press RET.

It just opens a new file named like "citekey nodeid". This makes sense because citar-open calls citar--open-multi, which has no idea what to do with a string returned by a note source's :items function. It just treats it like a filename and calls find-file on it.

[bdarcus]

Ok.

I don't know what happened. Could have sworn it worked on main.

But I'm kind of distracted ATM.

[bdarcus]

I just pushed a commit that looks to me like it should work, but doesn't. So it's close.

Can you take a look please?

[bdarcus]

I see the problem, though not yet a solution:

citar--select-resource returns a plain string; not a propertized multi-category string.

[bdarcus]

We might need an additional optional note property; a function to say what to do with the string?

But we still have no way of knowing it's a note, unless I guess we assume anything other than a URL or a file is?

Best if we remove my latest commit regardless; that won't work. done

[bdarcus]

This change also affects notes sources since the :hasnote and :items functions no longer get the entries argument. Instead, you can just use citar-get-entry, citar-get-entries, or citar-get-value, etc. if needed.

So you're no longer concerned about this?

[bdarcus]

Edit: I moved this to #653.

[aikrahguzar]

I understand the current practical need for such a solution, but at the same time I would be worried about losing the conceptual clarity that comes from knowing that the cache is only ever updated by calling parsebib-parse on a file. I would worry about subtle errors that make the cache diverge from the bibliography on disk.

I agree with this, those divergences would be hard to track down if they happen. And speed is definitely not a concern for me. The new cache generation is very fast and most of the my bib files have less than 200 entries so even the first run feels instantaneous.

[roshanshariff]

I think the best way to fix citar-open is:

1. citar--get-resource-candidates annotates all candidates with an extra citar--resource text property, which is either file, url, or note (instead of just annotating notes). We want to use a new property with no meaning for other packages, not reuse multi-category; we only use that if there are actually multiple categories.
2. citar--select-resource, after it gets the selected string from completing-read, looks up that string in the candidates list to get the propertized version, retrieves the citar--resource property, and returns that along with the string.
3. citar-open acts appropriately, calling citar-file-open, browse-url, or the note :open function as appropriate.

I think the :open function should just open notes, not create them; its argument should be a string returned by the :items function. What is now the :open function should be called :create, since it takes the citation key and entry and creates new notes.

Personally, I think this makes for a more logical notes API: :items returns a list of strings, any of which can be passed to :open. They can also be annotated with :annotate or transformed with :transform, but otherwise are not interpreted or modified in any way. And if you want to create a new note, you call :create.

As an aside, doing (1) also simplifies the grouping function a great deal, since it doesn't have to guess what type of resource it's been given.

I can implement this plan if it sounds acceptable.

[bdarcus]

Yes, that's acceptable; it's basically what I was thinking (the lookup part). Thanks!

[bdarcus]

I see @roshanshariff pushed the note fixes. Thank you!

So far, we haven't gotten any reported bugs or suggestions, except our own. And I believe we've resolved all.

Anything else that needs done before I merge, and tag 1.0?

[bdarcus]

I merged this. Will hold off on tagging 1.0 until I hear back.

[roshanshariff]

Would it be okay if we held off on tagging 1.0 for a week or two? There are some parts of the code base I would like to review more thoroughly. It's possible that a Citar 1.0 release will get some publicity, and it would be good to minimize the chances of bugs that might put off new users if they encounter them.

There are also a number of TODOS we should probably resolve as well. Maybe we could just do another point release in the meantime?

[bdarcus]

Yes; sure thing.

Should I go the point release now, or wait?

[roshanshariff]

Should I go the point release now, or wait?

I don't think we've introduced any serious new bugs, so I'd be okay with releasing now 😅

[bdarcus]

@roshanshariff I haven't had time to cut a release, but I did just assign a new bug to the 1.0 milestone.

I'm dealing with some crises ATM, so likely won't be able to help much.

But I raised your privileges, so you can merge things yourself.

[bdarcus]

Would it be okay if we held off on tagging 1.0 for a week or two? There are some parts of the code base I would like to review more thoroughly.

Where do we stand ATM @roshanshariff?

You obviously were able to address at least some of this with the notes functions, but I wasn't sure if there were other pieces of the code you felt needed review before we tag 1.0?

I know your time is limited these days, and mine will likely be for a couple/few months, so it's just a question of when the right time to pull the trigger is, without delaying too long.

From my POV, it certainly makes sense to close #507, but I think other issues can wait, though I did assign a few others to this milestone.

https://github.com/emacs-citar/citar/milestone/3

[aikrahguzar]

One thing I noticed while testing my recent changes on an old file: the new caching seems to be case-sensitive on the field names of the entry i.e. "Title" field is not equivalent to "title" now while it used to be. Maybe some uses of assoc-string string got replaced with something or no-longer have optional case-fold argument non-nil.

I don't know if this was intended and I don't think it is crucial since this can be resolved by adding both cased versions in the template. But seems like something that should be settled before 1.0 whatever the answer here is.

[bdarcus]

That sounds like an oversight.

It could be changes in parsebib maybe?

I do vaguely remember we needed to use assoc-string because of inconsistencies between bibtex and JSON as parsebib parses it (the former as keys as strings, and the latter as symbols).

It should remain the case that it works for both formats, as should ignoring case.

@roshanshariff?

[roshanshariff]

@aikrahguzar, thanks for spotting the case-sensitivity bug. In 64c332a, I've added back the case-fold option to the assoc-string in citar-get-value which got lost in the reshuffle.

@bdarcus, I think the main pending issue for 1.0 is the performance regression in #659. I did some preliminary investigation there and on my (fairly old) computer I don't see the problem right away (loads that bibliography in ~5 sec). I'll have to investigate a little more to see where the slowdown is.

Other than that, since we did the notes refactoring, I don't foresee any needed changes to external APIs in the immediate future. There's just the matter of mopping up the small bugs you've already tagged under the v1.0 milestone (and any others that might be uncovered in the meantime).

[bdarcus]

I did some preliminary investigation there and on my (fairly old) computer I don't see the problem right away (loads that bibliography in ~5 sec). I'll have to investigate a little more to see where the slowdown is.

That's really weird.

Maybe Colin could help with that?

I suppose in theory the PR I merged yesterday should speed things up a tad (since before that we were cleaning TeX strings twice), but not enough to explain what he's seeing.

If we can't reproduce it, though, that shouldn't hold up 1.0, since it doesn't appear to be a general problem, and a fix wouldn't impact the API.

[bdarcus]

Only question I have is if not using multi-category has implications for embark?

[roshanshariff]

It shouldn't make a difference to Embark, because from what I remember of its code, it doesn't distinguish between single-category completion and multi-category with all items having the same category.

It might make a difference to other hypothetical packages that interact with the completion system, since specifying a single category is part of the standard Emacs completion API, whereas the multi-category mechanism is a non-standard extension that is followed by Embark, Marginalia, etc.

[bdarcus]

Seems some sort of GitHub hiccup here. The comment says I posted that today. But I don't even remember doing that, which means it was probably months ago!

Shrug.