Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

[RFC] better project root detection. #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

roxma
Copy link
Owner

@roxma roxma commented Oct 17, 2017

I think this PR provides better project root detection, for most composer based php project.

It also works when you're openning a file without being in the project root. A typical use case would be openning files with MRU.

Note that this PR conflicts with #2 .

// cc @cmizzi @dantleech

@roxma roxma changed the title [RFC} better project root detection. [RFC] better project root detection. Oct 17, 2017
@dantleech
Copy link
Contributor

Would it make sense to expose a method in the Phpactor plugin phpactor#get_working_dir and use that?

@roxma
Copy link
Owner Author

roxma commented Oct 17, 2017

@dantleech

It make sense to me.

phpactor#get_working_dir should auto detect project root by default , since IMO it works for most composer based php projects.

@dantleech
Copy link
Contributor

dantleech commented Oct 17, 2017

should auto detect project root by default

It doesn't detect the project root, it just uses the directory in which VIM was initially loaded, I think there are too many edge cases when detecting the root (and the magic behavior can just be confusing).

I guess the other consideration is that this would couple this plugin to the phpactor.vim plugin, and I don't think is the case currently?

@roxma
Copy link
Owner Author

roxma commented Oct 17, 2017

I think there are too many edge cases when detecting the root (and the magic behavior can just be confusing).

In what case? phpactor is always depending on vendor/autoload.php generated by composer. And I haven't seen a PHP project using composer without putting vendor directory to the project root.

If it doesn't work by default, then the user need to configure it to work. But with a static value cwd=initialCwd by default, a user mostly need to re-configure it to work.

I guess the other consideration is that this would couple this plugin to the phpactor.vim plugin, and I don't think is the case currently?

I have already documented the requirements to the README page. It's not an issue.

@cmizzi
Copy link

cmizzi commented Oct 17, 2017

I like this PR. The project detection was removed from phpactor for a while. For example, when I'm using, I don't exit and re-create when switching projects. I just use MRU and I want to use phpactor to autocomplete any opened files (whatever if neovim was start in my $HOME or other directory).

If the detection fails for a project, a simple condition could be happen to the script detection in order to manage this case. I approve this PR because it's all I need right now to use this ncm plugin. It's not really different from my #2 , but this is more powerful and comprehensive.

I agree with @roxma to handle the project root detection method within the phpactor project (to also handle omnicompletion). This would be awesome !

@dantleech
Copy link
Contributor

In what case?

  1. If there is no composer, we traverse to the root of the FS, which is weird, a refactoring tool that does dangerous things shouldn't be in the root of your filesystem.
  2. If you are in a vendor within a vendor (i.e. you installed your deps in one of your dependencies for development), then you are not in the root of your project.
  3. ... ?

@cmizzi
Copy link

cmizzi commented Oct 17, 2017

You could take the parent .git root repository at this moment ?

@dantleech
Copy link
Contributor

dantleech commented Oct 17, 2017

You could take the parent .git root repository at this moment ?

More edge cases - what if the project doesn't use git? git submodules? (eventually Phpactor won't care which VCS you use, or even if you don't use one at all).

@roxma
Copy link
Owner Author

roxma commented Oct 17, 2017

If there is no composer, we traverse to the root of the FS, which is weird, a refactoring tool that does dangerous things shouldn't be in the root of your filesystem.

I get it. So it shouldn't be auto detected for phpactor.

If you are in a vendor within a vendor (i.e. you installed your deps in one of your dependencies for development), then you are not in the root of your project.

If you're in a sub project, it doesn't matter if you're editting sub-project's files and treating it as a project root.


So it's safe for code completion usage, but unsafe for refactoring usage.

But split them will break consistency between plugins.

@dantleech
Copy link
Contributor

If you're in a sub project, it doesn't matter if you're editting sub-project's files and treating it as a project root.

I'm not sure -- I think @cmizzi mentioned he was using an auto chdir plugin. So if editing a file in a dependency you would only get completion for that depdency, not the whole project (great for decoupling, but not realistic).

@cmizzi
Copy link

cmizzi commented Oct 17, 2017

I agree with @dantleech for this point of view. But, the solution could simply be to retrieve until latest found vendor or composer.json file. A loop to the root of the filesystem, for a plugin is not bad at all if the phpactor is not called on this path.

@roxma
Copy link
Owner Author

roxma commented Oct 17, 2017

hmm

I'm more concerned about the security issue mentioned by @dantleech

If there's no better solution. I will merge #2 instead, for security and consistency

@roxma
Copy link
Owner Author

roxma commented Oct 17, 2017

If there is no composer, we traverse to the root of the FS, which is weird, a refactoring tool that does dangerous things shouldn't be in the root of your filesystem.

Actually current PR doesn't traverse to the root of the FS. It fallbacks to getcwd() if composer is not found.

Further more, using a static var g:phpactorInitialCwd doesn't seem better than sticking with getcwd().

It's very common for vim users to :cd around. Keeping it consistent with vim's getcwd() is more straitforward and less confusing.

There're several vim plugins created for :cd around, e.g. vim-project

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants