Improve rails root finding

asok / projectile-rails

Emacs Rails mode based on projectile

258 stars 59 forks source link

Improve rails root finding #113

Closed shanavas786 closed 7 years ago

shanavas786 commented 7 years ago

Check for projectile-rails-root-file only if projectile-project-root is not rails root.

Use configurable filenames to identify and verify rails root

Silex commented 7 years ago

Is it really faster? I mean, projectile-project-root uses locate-dominating-file already.

The variablafication of the paths is nice tho.

shanavas786 commented 7 years ago

Oh, I see. It uses some caching however.

Silex commented 7 years ago

Yeah I'm not against your changes, I'm just curious about the "faster" part and in what scenario the slowness of the current method is actually perceptible. Maybe over TRAMP?

shanavas786 commented 7 years ago

I thought projectile-project-root would be more optimal and we could reuse it. Now I realize that doesn't make much difference in performance.

Silex commented 7 years ago

When doing something for performance reasons, always measure :wink:

Also, at the start of the project we did only use projectile-project-root, but then projectile-rails would fail in repositories where rails was in a subdirectory. That's why we changed for the locate-dominating-file version.

See https://github.com/asok/projectile-rails/issues/68 and https://github.com/asok/projectile-rails/pull/69/commits/3c035ee1c6da77384a406469a6307e511654c6f7 for the history.

@asok: I'll let you decide wether the variablafication of the paths should be included or not. I don't think it hurts and allows more customization.

asok commented 7 years ago

Thanks for your contribution. It feels OK to have this customizable. Two things though:

maybe instead of projectile-rails-config-file name a better one would be projectile-rails-verify-root-file?
it makes more sense for those variable to be per project. Any ideas how to accomplish that? Maybe by making them a local variables and setting them up in the .dir-locals.el file?

shanavas786 commented 7 years ago

Thanks, I had to work on some rails projects which doesn't have a Gemfile at all (needs a fix of course :smile: ). In such cases, it helps to use some other files to serve the purpose. May be Rakefile or .git itself.

It could be set globally in dot file or in directory locals specific to a project.

asok commented 7 years ago

I also had in mind to make the variables buffer local. So if you set it in one project it is set only for that project and for others it remains the default value.

Any ideas how to combine it with .dir-locals to achieve such functionality?

shanavas786 commented 7 years ago

won't

((projectile-rails-mode . ((projectile-rails-root-file . "some file")
                                       (projectile-rails-verify-root-file . "another one"))))

do ?

asok commented 7 years ago

I just consulted the manual and it says the dir-locals makes the file buffer local so it's safe to set them that way.

Silex commented 7 years ago

Hum, next time consider squashing the commits as one commit for PRs :wink: