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

Support windows type builds in the guessprojectroot function #112

Closed
wants to merge 10 commits into from

Conversation

Numkil
Copy link

@Numkil Numkil commented Sep 29, 2015

Probably fixes #111. @codeur4 could you check out this PR for me and try it out? I don't have a windows pc in my possession.

@codeur4
Copy link

codeur4 commented Sep 29, 2015

No, same as before

@Numkil
Copy link
Author

Numkil commented Dec 15, 2015

@tplunket thanks for taking the time to try and fix this. I just didn't have the time to look into it. Your code looks good to me!

@codeur4 Would you be so kind in testing this PR again?

@tplunket
Copy link

My pleasure, I also happened to have a Windows machine. ;-)

@hunhejj
Copy link

hunhejj commented Jan 27, 2016

@Numkil I tested it and it won't work yet for Windows if the project is in the root drive.
It works only after these two modifications:

  1. trailing slash should be added when returning in line 231
  2. it should look for len(l:searchdir)>1 in line 225 (because later a slash is added in line 228)

@Numkil
Copy link
Author

Numkil commented Jan 27, 2016

@hunhejj Thank you for helping out, I will see into these changes.
@tplunket could you try out the new lines of code I will add? Just to make sure it keeps working on everyone elses machine.

@tplunket
Copy link

Returning a trailing slash from s:guessProjectRoot is unnecessary. The only thing it's used for is the argument to 'lcd'.

There's a comment documenting why len(l:searchdir) is checked to be greater than 2; that was basically the previous behavior but on Windows it doesn't allow you to have an entire drive under version control. Any folder at root should still be searched, e.g. C:\A is a directory name that would pass the test. I don't see where the later slash is added; in the PR line 228 is a comment.

I've just reworked that logic though to unify it between Windows and POSIX systems. There's a PR waiting for Numkil to evaluate. :) (Didn't see Numkil's message before starting.)

@hunhejj
Copy link

hunhejj commented Jan 27, 2016

You can have an entire drive under version control, if you map a folder to a drive (this is my case).

I didn't dig deep into the code, however with the above 2 changes it works, and without any(!) of them, it won't, therefore the trailing slash is somehow important.

@tplunket
Copy link

Try to map the version in my repo. According to what Numkil posted earlier,

Plug 'tplunket/ag.vim', { 'branch': 'fixGuessProjectRootOnWindows' }

I don't use any plugin managers either but handles going all of the way to the root of the filesystem on both Windows and POSIX.

@tplunket
Copy link

So that I could debug this, I put

echo 'Trying' l:item

right above the "if filereadable" line, then

call input('Found it in ' . l:searchdir)

right before "return l:searchdir" and another 'call input' right before the failure case return at the end of the function. If it's still not working maybe these will help us figure out why.

I actually have some code to allow you to provide your own function for the searching (in my master branch) and have pondered allowing you to specify the list of things to look for (e.g. I always have a file named 'tags' in the project root) but haven't been motivated to do the documentation to finish them up.

Tighten up guessProjectRoot for Windows
@Numkil
Copy link
Author

Numkil commented Jan 27, 2016

I merged @tplunket's code in my pull request. It's working fine for me on all platforms. (linux, osx, windows).

@tplunket
Copy link

@hunhejj due to the way Windows directory changing works, yes, the trailing slash is required at the root... Ugh, ok I'll get another patch in for that. I hadn't thought of testing it with a subst'd drive.

@tplunket
Copy link

...and pondering it, the slash needs to be added for POSIX root too, otherwise it'll try to lcd with no argument.

…nfo.

Fixes changing to root directory if the entire volume is under source control.
@tplunket
Copy link

@hunhejj give it a shot now, from my repo if you don't want to wait for @Numkil to pull.

@hunhejj
Copy link

hunhejj commented Jan 28, 2016

@tplunket i pulled your branch, but it still doesn't work because of the missing slash.
Just try :lcd c: on a windows vim (returns the error "can't find direcotry "c:" in cdpath"). Adding the slash before returning really solves the problem.

Returns a trailing slash on the directory name holding project root
@Numkil
Copy link
Author

Numkil commented Jan 28, 2016

I merged this as well. @hunhejj @tplunket I guess it should be ready now :)

@hunhejj
Copy link

hunhejj commented Jan 28, 2016

@Numkil nice, thanks!

@tplunket
Copy link

@hunhejj I added the slashes, or at least I tried to... Any feedback on what the failure path in the code is?

@tplunket
Copy link

@hunhejj I did it on Mac and just now tried it on Windows, subst'ing a drive, and it switched directories to the root just fine.

@Numkil
Copy link
Author

Numkil commented Jan 28, 2016

Tried it myself and it does work for me too.
@hunhejj If you could verify if everything works as expected now that would be great.
@losingkeys This pull request should be almost ready. Could you merge it when @hunhejj verifies it works?

@hunhejj
Copy link

hunhejj commented Jan 28, 2016

@hunhejj, @tplunket, @losingkeys Absolutely, it works perfectly on Windows. That is why I thanked for your solution in my last comment.

@Numkil
Copy link
Author

Numkil commented Apr 11, 2016

Hey @losingkeys this pr has been finished for a while now and it's necessary to remove an existing bug from the project. Could you look into this?

@losingkeys
Copy link
Collaborator

@Numkil yeah I'll try to test it out on my windows box soon here. Thanks for all your hard work!

@Numkil Numkil changed the title [WIP] Support windows type builds in the guessprojectroot function Support windows type builds in the guessprojectroot function Jun 2, 2016
@hunhejj
Copy link

hunhejj commented Jul 19, 2016

@losingkeys any progress?

@Numkil
Copy link
Author

Numkil commented Jul 19, 2016

@hunhejj. @losingkeys deprecated ag.vim completely and said he won't be working on it anymore again. so we will likely never see it merged...

@tplunket
Copy link

tplunket commented Jul 19, 2016

I'm not sure where @Numkil's various branches are but @hunhejj if you want to pull from my master (https://github.com/tplunket/ag.vim) branch, it's only missing the deprecation notice and some help text about a 'ga' motion that doesn't seem to have been implemented. I'll keep fixing any issues that come up over there and I'm not going to fall victim to that weird situation that they created for themselves in the original repo.

@hunhejj
Copy link

hunhejj commented Jul 19, 2016

oh, I wasn't aware that he deprecated his project. Thank you @tplunket for the hint! Actually I have a working version already, I was just courious why it was never merged. But now I understand.

@losingkeys
Copy link
Collaborator

@hunhejj, @Numkil, @tplunket: sorry if it wasn't clear. I didn't want to close the issues in case people wanted to migrate them. Let me know if the README could have more clarification as to why this has been deprecated.

@Numkil Numkil closed this Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

let g:ag_working_path_mode = 'r' doesn't work
5 participants