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

Ensure that multicite links are correctly split #545

Merged
merged 7 commits into from
May 1, 2020

Conversation

TimQuelch
Copy link
Contributor

org-ref multi-citations are handled by "cite:key1,key2". In the org-roam databases this was
previously stored as a single "cite" link to the (non-existing) key "key1,key2". This means that
cite backlinks and cite graph behaviour does not work correctly for these links.

This fix will split any cite links with a comma into separate links to each individual ref.

To be honest, I don't really like how this patch changes the readability of org-roam--extract-links. Because I've basically wrapped the existing code in a mapccan, the code to split the multicite links now appears before the code to actually extract the links; in my mind these should be the other way around. In addition, properly indenting would cause a gross diff (I've currently left the original indentation). Not sure how to fix this with my limited emacs-lisp knowledge so suggestions are welcome.

Motivation for this change

Fixes #544

org-ref multi-citations are handled by "cite:key1,key2". In the org-roam databases this was
previously stored as a single "cite" link to the (non-existing) key "key1,key2". This means that
cite backlinks and cite graph behaviour does not work correctly for these links.

This fix will split any cite links with a comma into separate links to each individual ref
@zaeph zaeph requested review from zaeph and removed request for zaeph May 1, 2020 06:18
@zaeph zaeph self-assigned this May 1, 2020
org-roam.el Outdated Show resolved Hide resolved
@zaeph
Copy link
Member

zaeph commented May 1, 2020

To avoid the long argument in mapcan, I've pushed a slight refactoring with ->>. Make sure to pull!

@TimQuelch
Copy link
Contributor Author

To avoid the long argument in mapcan, I've pushed a slight refactoring with ->>. Make sure to pull!

Oh nice, that's exactly what I wanted. I didn't know about the dash library, it looks very useful

@TimQuelch TimQuelch marked this pull request as ready for review May 1, 2020 07:40
@TimQuelch TimQuelch requested a review from jethrokuan May 1, 2020 07:40
@@ -9,6 +9,7 @@
* [#509](https://github.com/jethrokuan/org-roam/pull/509) fix backup files being tracked in database
* [#509](https://github.com/jethrokuan/org-roam/pull/509) fix external org files being tracked in database
* [#537](https://github.com/jethrokuan/org-roam/pull/537) quote graphviz node and edge configuration options to allow multi-word configurations
* [#545](https://github.com/jethrokuan/org-roam/pull/545) fix `org-roam--extract-links` to ensure that multiple citations (`cite:key1,key2`) are split correctly
Copy link
Member

@zaeph zaeph May 1, 2020

Choose a reason for hiding this comment

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

Way more diligence than I showed in any of my PRs right there. 🤡

Thanks!

Copy link
Member

@jethrokuan jethrokuan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zaeph zaeph left a comment

Choose a reason for hiding this comment

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

I think we could do a little improvement with the conditions.

org-roam.el Outdated Show resolved Hide resolved
org-roam.el Outdated Show resolved Hide resolved
@zaeph
Copy link
Member

zaeph commented May 1, 2020

Welp, the code might be more readable, and I'd certainly deserve some golf-points for it, but it isn't that much more optimised:

;; Original
(with-selected-window (next-window)
  (benchmark 100 '(org-roam--extract-links)))
;; "Elapsed time: 5.322224s (1.574320s in 17 GCs)"

;; Optimised...?
(with-selected-window (next-window)
  (benchmark 100 '(org-roam--extract-links)))
;; "Elapsed time: 5.352579s (1.565723s in 17 GCs)"

I guess it'd be better if we could get org-element-map to not accumulate the results, but unless we send a patch upstream (i.e. Org-mode) to get it to accept a new argument, it's not going to happen.

Oh well. ¯\_(ツ)_/¯

@zaeph zaeph merged commit 1ad3539 into org-roam:master May 1, 2020
@zaeph
Copy link
Member

zaeph commented May 1, 2020

Thanks for your work on this!

@TimQuelch TimQuelch deleted the multi-cite-links branch May 1, 2020 10:00
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.

Multiple org-ref citations are not recognised as multiple links
3 participants