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

Fix for ObsidianOpen not working with links #113

Merged
merged 9 commits into from
Mar 31, 2023
Merged

Fix for ObsidianOpen not working with links #113

merged 9 commits into from
Mar 31, 2023

Conversation

shakesbeare
Copy link
Contributor

@shakesbeare shakesbeare commented Mar 28, 2023

Potential fix for #107

Summary

This change modifies the ObsidianOpen command to additionally check if Plenary's make_relative method fails by attempting to find the vault root directory in the path after the call to make_relative. If the vault root directory is found, the new implementation simply substitutes out everything up to and including the root directory in an attempt to make the path work.

Limitations

  • This method will not work if the link which the vault is behind is directly to the vault and the name of the link does not match the name of the vault. In this case, both make_relative and my proposed fix will fail. To fix this would require actually detecting if the vault is behind a link and following it in order to initialize the vault path as its absolute version from the get go. The only way I could find to accomplish this was to use an external library for Lua, which I figured might not be ideal to require as a dependency for what is really a bit of a niche bug.
  • This will also fail if the user happens to keep their note in a subfolder of their vault which shares the same name as the vault itself. This seemed like a more unlikely scenario than someone wanting their vault directory to be behind a link, which would justify the change.
  • I believe I have accounted for Windows paths as well, though I imagine that links will look and function differently in Windows. This change may not work for Windows users as a result. That said, it shouldn't break any existing instances that were already working.

Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @shakesbeare! Could you just add some comments in the code to explain what's going on and why we have to do that?

@shakesbeare
Copy link
Contributor Author

@epwalsh Done! Plus, I added a Changelog entry. I was toying with the idea of mentioning the issue somewhere in the README, but wasn't sure how to phrase it given that it should work properly for most everyone now. Thoughts?

@epwalsh
Copy link
Owner

epwalsh commented Mar 30, 2023

I was toying with the idea of mentioning the issue somewhere in the README, but wasn't sure how to phrase it given that it should work properly for most everyone now.

I like the idea of adding a "## Known issues" section, and I think saying something like "this is edge case that won't affect most people" is fine.

Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

LGTM!

I'll hold off on merging until you let me know if you want to add something to the README about known issues.

@shakesbeare
Copy link
Contributor Author

@epwalsh Went ahead and added the README section

@epwalsh epwalsh enabled auto-merge (squash) March 31, 2023 18:24
@epwalsh epwalsh merged commit 09a0a36 into epwalsh:main Mar 31, 2023
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.

2 participants