Coding style for org-roam-bibtex

[myshevchuk]

@Zaeph
There are a couple of general emacs-lisp style guides, most notably the official one: https://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html and another one here: https://github.com/bbatsov/emacs-lisp-style-guide

I would suggest we adhere to the guidelines outlined in the above resources. Which we seem to have been doing so far anyway.

I have started this issue, however, with subtler matters in mind, which it seems did not make it into the guides, and which will likely arise in the development process.

In the recent commit 715083d I have introduced .dir-locals.el and also a file-local variables section at the bottom of org-roam-bibtex.el.

So far, sentence-end-double-space is set to t as a top-directory variable (Emacs default). It seems my Emacs installation has it set to nil and I have constantly been messing up the spaces in the doc-strings - I would like to apologise for that.

I also set fill-column to 70 , which is Emacs default, as a file-local variable.

If you have other preferences, please feel free to change these and other relevant variables to your liking. I care as much as to keep the things coherent, the actual values being of lesser importance.

[zaeph]

So far, sentence-end-double-space is set to t as a top-directory variable (Emacs default). It seems my Emacs installation has it set to nil and I have constantly been messing up the spaces in the doc-strings - I would like to apologise for that.

No problem whatsoever, don't sweat it.

So far, sentence-end-double-space is set to t as a top-directory variable (Emacs default). It seems my Emacs installation has it set to nil and I have constantly been messing up the spaces in the doc-strings - I would like to apologise for that.

I also set fill-column to 70 , which is Emacs default, as a file-local variable.

Sounds good. I'm ironing it the kinks as I see them, but I'm learning as I'm going, just like you I assume. On top of the links you've mentioned, there's also @alphapapa's excellent Emacs Package Developer's Handbook.

Now that the README.md is a little more furnished, I'm focussing on testing. I'm learning the ropes of emacs-buttercup. It's finals week for me, though, so there probably won't much work done on the project on my end this week.

And thanks again for your help maintaining the package. I think we've done pretty good so far! 🚀

[myshevchuk]

Yes, thank you very much for you collaboration! Indeed, everything is developing very nicely.

[myshevchuk]

Here I would suggest that we do not require our feature libraries in org-roam-bibtex.el, except for orb-compat.el of course. Currently, we have only orb-note-actions.el, which has only one user function orb-note-actions, which has the autoload magic comment. Normally, when a package is installed through a package manger such as package.el, use-package or straight.el, the autoloads file is generated automatically.

There are potentially two problems with this paradigm.

1. user variables (customize definitions) are not discoverable until the package is loaded, that is until the autoloaded function is called.
2. autoloads, of course, are not generated automatically if the main file org-roam-bibtex.el is loaded with load-file rather than a package manager.

The latter problem can be addressed easily by providing instructions in README on how to generate autoloads file. The former surely can also be solved but I need to research how to do this correctly.

We may also have in future some auxiliary library or libraries that would contain purely utility functions akin to orb--unformat-citekey, which could be potentially used across several "functionality" or feature libraries. Those, of course, should be required in org-roam-bibtex.el. Currently, there is only one such file, org-compat.el mentioned earlier.

So the structure would be like this:

utility libraries -> org-roam-bibtex.el -> feature libraries

The arrows show how the features are provided. Utility libraries provide features to org-roam-bibtex.el, which requires them. Feature libraries require only org-roam-bibtex.el, which will also provide the features provided to it by the utility libraries.

[zaeph]

So the structure would be like this:

utility libraries -> org-roam-bibtex.el -> feature libraries

Sounds good.

I think I got confused because I couldn't get my Emacs to load the library at some point, but that might have been due to an unrelated error. I might have been using the wrong session to run my tests, one which hadn't loaded orb-note-actions.el, or maybe I screwed up and didn't reload the file upon checkout. At any rate, sorry for that.

And so, this understanding led to #25, which led to a recursion error spotted by #27, and fixed in 8cc2d954d3ae753d882f349533ce3b51808475c3.

I think that the bulk of the confusion came from #26: where to define the key-binding? In the org-roam's use-package? In a separate, parallel use-package just for orb-note-actions? Or one nested in org-roam's?

2. autoloads, of course, are not generated automatically if the main file org-roam-bibtex.el is loaded with load-file rather than a package manager.

That is such an edge-case that I'm not even sure if it bears mentioning

[zaeph]

#30 reverts f0a65ae9b9fd779835dbea2b5ebfa40657f7d209. I'll let you merge once you've confirmed this is what you wanted.

[zaeph]

Also, I'm crunching for my MA thesis right now, so I'm going to take the week off to focus on it. I'll try to keep an eye out on the repo, but I won't be doing anything substantial.

[myshevchuk]

There is nothing principally wrong in requiring orb-note-actions in the main file. Autoloading it is a tiny optimization - tiny considering the current size of the library - that is inline with the current trend to autoload everything. I'm using doom-emacs, which takes care of installing the packages and autoloading them (via straight), so the previous version worked fine for me. I haven't tested it with a bare minimum though. I think at some point we'd need to set an automated test environment, but that is a long-standing goal I guess. I'm currently busy with my PhD thesis and am not currently predisposed to engage in this type of work.

If you install org-roam-bibtex with MELPA and use use-package, the setup should be as simple as this, essentially as described in the README but without the extra parentheses around ("C-c n a" . orb-note-actions) (maybe this was the cause of the problems?)

(use-package org-roam-bibtex
:after org-roam
:bind (:map org-mode-map
         ("C-c n a" . orb-note-actions))
:hook (org-roam-mode . org-roam-bibtex-mode)
:config
(setq ...))

Could you please test the latest commit of this branch with the setup above? No need to hurry though, this can wait.

[zaeph]

Could you please test the latest commit of this branch with the setup above? [emphasis added]

Do you mean #30 or master?

[myshevchuk]

#30 with my commit.

[zaeph]

#30 with my commit.

It's working. I'm merging.

[myshevchuk]

Ok, good.

[zaeph]

Also, I've disabled the option to merge all commits from a PR, and instead, we're going to squash-then-merge them. That'll prevent a lot of small commits from crowding up the history.

[myshevchuk]

YES!

[myshevchuk]

Now, enough procrastinating. It's working, it's doing its job. And there is real work ahead to be done.

[zaeph]

Righto. Glad to know you're in the spot as I am, although a bit further down the line.

Let's reconvene once we've done enough progress to convince our supervisors that we're doing decent work. 🤡

[myshevchuk]

I've created a development branch develop, which is not for merging with master but for testing purposes. All the new branches should first go into it and then into master. It would also make it more convenient to test different features together, since different feature branches can be developed independently and are not always supposed to be merged one into another. They instead should be merged into this branch.

New features should start on master and then merged back into it after being tested on develop.

[zaeph]

Sounds good. I did it in a next branch on my own fork, but develop works just as well.