search

dajva / rg.el

Emacs search tool based on ripgrep
https://rgel.readthedocs.io
GNU General Public License v3.0
474 stars 39 forks source link

Add support for root detection via built-in project.el #51

Closed wedens closed 5 years ago

wedens commented 5 years ago

Works with emacs >= 26

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.1%) to 82.766% when pulling 7bc483b222973e72151c5c7fd5d73379651ab9d9 on wedens:patch-1 into 2e4aae5af88659f70e693aad6c26c8ac9025d60e on dajva:master.

dajva commented 5 years ago

Thanks a lot. Didn't know this existed.

Could you also add a test for it in rg-integration-test/project-root between the ffip and vc-backend test? I think this should do it:

;; project.el
(rg-check-git-project-root)
(eval-after-load 'project
  (fmakunbound 'project-current))
wedens commented 5 years ago

@dajva I noticed commented test for projectile with a comment:

  ;; projectile require emacs 25.1 so can't test with cask since we
  ;; support emacs 24.4.

and package.el requires emacs >= 26. So, I wasn't sure I can test it.

I've tried to add the test, but CI is still failing and I'm not sure why

dajva commented 5 years ago

It should be fine testing this in versions, not supporting project.el. The test structure is a bit peculiar for this test but what will happen in that case is the vc-backend version will be invoked twice which is ok.

I think the problem actually lies in your patch. I looked at the project.el code and seems it doesn't return a string as all the other backends, but a cons cell of (symbol . dir-as-string). I think (you'll have to test this) that you need to use (cdr (project-current)) in you original patch.

dajva commented 5 years ago

(The problems with projectile lies entirely within cask)

wedens commented 5 years ago

Yeah, I forgot to add cdr 🤦‍♂️

CI is still failing for whatever reason. I don't have Cask, etc installed locally, but if I just call rg-project-root it seems to work as expected.

dajva commented 5 years ago

Ok, you can leave the tests. I'll have a look at this and then merge it.

dajva commented 5 years ago

Ok, this was entirely my fault. I used eval-after-load in the wrong way which accidentally worked for the other packages but not for project.el for some reason. I have merged this pr and pushed a fix for the eval-after-load problems.

Thanks