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

feat(links)!: Use refactor links structure #803

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Conversation

kristijanhusak
Copy link
Member

@kristijanhusak kristijanhusak commented Aug 23, 2024

Start using new links structure introduced in #802.

@chipsenkbeil can you check if using this branch breaks anything in org-roam.nvim? It shouldn't, but it's best to double check.
You will need to introduce one change here: https://github.com/chipsenkbeil/org-roam.nvim/blob/192732a8de167e7770b12db31e737239311b1a43/lua/org-roam/setup/plugin.lua?plain=1#L20

I added a hyperlink class in

local OrgHyperlink = {}
, and it works basically the same for your usage. Underlining url prop is also a different class, but it has the get_id method that you are using.

For this line you can use Hyperlink.at_cursor function instead of these 3 lines to get current link.

Besides this I think everything should be working as expected, but please give it a thorough test.

@seflue @SlayerOfTheBad could you switch to this branch and see if everything related to hyperlinks works as expected?

Thanks!

@kristijanhusak
Copy link
Member Author

@chipsenkbeil @seflue @SlayerOfTheBad mentioning just in case you didn't receive the notification, since I mentioned you in an edit of a description.

@chipsenkbeil
Copy link
Contributor

@kristijanhusak thanks for keeping me in the loop :) I got your first mention, but glad you double-checked.

I can get this change into the plugin in the next day or two. Is this going to mark a new release? Wondering if I should

  1. Mark a new release that locks to a new version of the orgmode plugin so I can change the code to use the update
  2. Or, update my code to try to use the new method and fall back to the old approach if it doesn't exist, thereby supporting both changes. Would still mean that folks using older versions of my plugin would break if they updated to your latest

Leaning towards the former if you're planning to mark this as a new orgmode release 😄

@kristijanhusak
Copy link
Member Author

kristijanhusak commented Aug 23, 2024

I put out a release 0.3.5 yesterday that does not have any of these link changes. You can pin your current version to this version.

Once i merge this, i can tag another one, but I'd prefer to iron out any bugs before doing that.

Edit: Note that your current code should not break even on this PR since i didn't remove old code, but please double check.

@chipsenkbeil
Copy link
Contributor

Edit: Note that your current code should not break even on this PR since i didn't remove old code, but please double check.

Ah, okay! I thought this was a callout to breaking change. If this is not breaking then even better.

I think I'd like to wait towards making the change until after the tag, but I can create a branch of my plugin with the change to verify that it works with the new code. I just won't merge it until I can tag a new release on my side that syncs up with yours.

So TLDR, I'll help verify that things work and the change behaves as expected, but will wait to land until after it rolls out in a tagged orgmode release.

@seflue
Copy link
Contributor

seflue commented Aug 23, 2024

@chipsenkbeil @seflue @SlayerOfTheBad mentioning just in case you didn't receive the notification, since I mentioned you in an edit of a description.

@kristijanhusak Thanks for the reminder. I will check, if I need to adjust something in the next days and update you here.

@kristijanhusak
Copy link
Member Author

@chipsenkbeil @seflue did you had a chance to test this? If yes, any issues found?

@kristijanhusak
Copy link
Member Author

I'm merging this in. We can solve any issues as we go. Org-roam suggests using 0.3.4 version so we should be ok.

@kristijanhusak kristijanhusak merged commit c5940d3 into master Sep 18, 2024
6 checks passed
@kristijanhusak kristijanhusak deleted the feat/new-links branch September 18, 2024 10:30
@chipsenkbeil
Copy link
Contributor

@kristijanhusak all good to merge since I lock versions.

Haven't set aside time to check. Will make a note to try to do so this week.

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.

3 participants