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

Compare resolved paths in #GetOrInstall() #23

Merged
merged 2 commits into from Dec 30, 2013
Merged

Compare resolved paths in #GetOrInstall() #23

merged 2 commits into from Dec 30, 2013

Conversation

glts
Copy link
Contributor

@glts glts commented Dec 29, 2013

This fixes #18 for me. I don't have vroom to test this.

I understand if you prefer a different approach. Anyway, I've signed the CLA in the past so you can merge or roll your own solution as you please :)

@@ -320,10 +320,13 @@ function! maktaba#plugin#GetOrInstall(dir, ...) abort
let l:settings = maktaba#ensure#IsList(get(a:, 1, []))
if has_key(s:plugins, l:name)
let l:plugin = s:plugins[l:name]
let l:fullpath = s:Fullpath(a:dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

s:Fullpath(a:dir) went away. About s:Fullpath(): I'm not sure what purpose it serves. The comment in s:Fullpath() says "The #Join ensures a trailing slash", but fnamemodify(a:location, ':p') already guarantees a trailing slash for paths ending in a directory name!

Copy link
Contributor

Choose a reason for hiding this comment

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

l:plugin.location is the result of a call to s:Fullpath(), so it's probably a good idea to still call it for l:newpath for parallelism even if we can intuit that it's not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, just so I understand correctly: use s:Fullpath(a:dir) and then call fnamemodify() with :p:h on it. I will do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that's what I had in mind.

And be sure to use the paths with trailing slash in the error message.
@dbarnett
Copy link
Contributor

LGTM.

dbarnett added a commit that referenced this pull request Dec 30, 2013
Compare resolved paths in #GetOrInstall()
@dbarnett dbarnett merged commit 2da895d into google:master Dec 30, 2013
@glts glts deleted the resolve-plugin-paths branch December 30, 2013 19:25
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.

Symlinked plugin location results in AlreadyExists conflict
2 participants