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

refactor: Default Org front-matter to use the ID property #20

Merged
merged 3 commits into from Jun 24, 2022

Conversation

kaushalmodi
Copy link
Contributor

@kaushalmodi kaushalmodi commented Jun 23, 2022

  • Move denote-use-org-id from denote.el to denote-link-use-org-id in denote-link.el. The rationale for this change is that the ID property style will always work for Org files and doesn't need any customization; also that style doesn't add any org-id dependency.
  • The org-id dependency is needed only if the user wants to access "active" feature of the "id:" type links like following and exporting.

Fixes #8.

protesilaos and others added 2 commits June 23, 2022 10:42
For a discussion, read issue 8 on the GitHub mirror:
<protesilaos#8>.
Move `denote-use-org-id` from denote.el to `denote-link-use-org-id` in
denote-link.el. Rationale for this change is that the ID property
style will always work for Org files and doesn't need any
customization; also that style doesn't add any org-id dependency.

The org-id dependency is needed only if the user wants to access
"active" feature of the "id:" type links like following and exporting.
@kaushalmodi
Copy link
Contributor Author

I have one suggested change on top of your org-id branch. I have rebased that to main and added a commit on top of that in this PR.

I tested out that using the ID property by default in Org file works well, and doesn't have any org-id dependency. But if the user wants to use org-id, they would already have an Org ID compatible ID property 😃 . So I didn't see any harm in leaving that as the default.

I additionally tested creating Markdown files and linking to the Org files with ID property and the denote: links in the Markdown files got created correctly.


I did find one issue in the org-id branch which needs to be fixed separately. With the new denote-link-use-org-id set to t (or the old denote-use-org-id set to t), the id: links should be inserted only if both target and origination note files are Org files. Right now, if the origination file (where the link is inserted), the denote: link is correctly inserted. But if the target link is not Org, then also denote: link needs to be inserted, even if denote-link-use-org-id is set to t.

Example: A user linking a Markdown note from an Org note.

@protesilaos
Copy link
Owner

So I didn't see any harm in leaving that as the default.

Fine by me and it does make sense. Just how we use a different date format for Markdown to ensure compliance.

I did find one issue in the org-id branch which needs to be fixed separately.

Indeed. This is what I alluded to in issue 8: #8 (comment)

Do you want to do it as part of this PR? Do you wan to do it separately? Shall I give it a try?

Just asking to better coordinate our work.

Thanks for doing this and for the feedback in general!

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 23, 2022

Shall I give it a try?

I quickly looked for a way to check the target file type and origination file type, but couldn't find it. I won't have time to work on that today. So can you please add a fix for that?

Thanks for doing this and for the feedback in general!

Thanks to you! My effort in all of this is just like a droplet in an ocean :).

@protesilaos
Copy link
Owner

I quickly looked for a way to check the target file type and origination file type, but couldn't find it. I won't have time to work on that today. So can you please add a fix for that?

Sure!

Now I just need to figure out how best to incorporate your changes into the org-id branch so I can add the final bits. I am not too familiar with the PR workflow...

@protesilaos
Copy link
Owner

Now I just need to figure out how best to incorporate your changes into the org-id branch so I can add the final bits. I am not too familiar with the PR workflow...

Okay, I think I found it. Will pull from here: git@github.com:kaushalmodi/denote.git

@protesilaos
Copy link
Owner

Merged here 34554a7. But I do not know how to mark this as "merged".

@kaushalmodi
Copy link
Contributor Author

As I have submitted a PR, you can fetch that branch directly from your own git remote as well.

e.g.

git fetch origin pull/20/head:pr-20

This will create a local branch pr-20 synced up with the then latest commit in this PR (PR 20).

Further reading

@kaushalmodi
Copy link
Contributor Author

But I do not know how to mark this as "merged".

That's alright, you can then just close it (I'll close it).

Thanks!

@kaushalmodi kaushalmodi reopened this Jun 23, 2022
@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 23, 2022

Related to viewing PRs in Emacs, I found this neat trick by Oleh Krehel (developer of ivy/counsel/etc.): https://oremacs.com/2015/03/11/git-tricks/

In the denote repo's .git/config, I changed from:

[remote "origin"]
    url = https://github.com/protesilaos/denote
    fetch = +refs/heads/*:refs/remotes/origin/*

to

[remote "origin"]
    url = https://github.com/protesilaos/denote
    fetch = +refs/heads/*:refs/remotes/origin/*
    fetch = +refs/pull/*/head:refs/pull/origin/*

Now, in Magit, when I do f a (fetch all), I see

image

@kaushalmodi
Copy link
Contributor Author

btw I didn't understand what the 34554a7 merge commit does. This PR branch is already rebased to the latest origin/main.

@protesilaos
Copy link
Owner

btw I didn't understand what the 34554a7 merge commit does. This PR branch is already rebased to the latest origin/main.

I must have messed up then. Should I revert?

@kaushalmodi
Copy link
Contributor Author

It looks like that commit is only on your remote's org-id branch, not the main branch. You can check out a branch starting from 80eb752, add additional commits for the org-id fix and commit that to main.

@protesilaos
Copy link
Owner

It looks like that commit is only on your remote's org-id branch, not the main branch.

Yes, that was the idea so that I can add the final part about checking the file type of the target file. Will it be wrong that way?

@kaushalmodi
Copy link
Contributor Author

Will it be wrong that way?

"Wrong" might be subjective.. it's just that the merge commit seemed unnecessary since this PR is already rebased to the main branch of this repo.

@protesilaos
Copy link
Owner

t's just that the merge commit seemed unnecessary since this PR is already rebased to the main branch of this repo.

Okay. What's the best way then to continue working on a separate branch before merging everything back into main? Shall I check out 80eb752 as you explained?

Or shall I just merge this PR and continue working from main?

@kaushalmodi
Copy link
Contributor Author

Shall I check out 80eb752 as you explained?

👍

Or shall I just merge this PR and continue working from main?

This is an equally good option.

@protesilaos
Copy link
Owner

Starting working from 80eb752. More to follow. Thanks!

@protesilaos
Copy link
Owner

Made the changes and merged everything into main. The branch I was working on, if needed: https://github.com/protesilaos/denote/tree/org-id-links

@protesilaos protesilaos merged commit 80eb752 into protesilaos:main Jun 24, 2022
@kaushalmodi kaushalmodi deleted the org-id branch June 24, 2022 03:28
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.

Canonical (probably?) way to set file ID in Org
2 participants