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

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

Closed
wants to merge 3 commits into from

Conversation

wedens
Copy link
Contributor

@wedens wedens commented Dec 16, 2018

Works with emacs >= 26

@coveralls
Copy link

coveralls commented Dec 16, 2018

Coverage Status

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

@dajva
Copy link
Owner

dajva commented Dec 16, 2018

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
Copy link
Contributor Author

wedens commented Dec 16, 2018

@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
Copy link
Owner

dajva commented Dec 16, 2018

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
Copy link
Owner

dajva commented Dec 16, 2018

(The problems with projectile lies entirely within cask)

@wedens
Copy link
Contributor Author

wedens commented Dec 16, 2018

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
Copy link
Owner

dajva commented Dec 16, 2018

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

@dajva
Copy link
Owner

dajva commented Dec 16, 2018

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

@dajva dajva closed this Dec 16, 2018
@dajva dajva reopened this Dec 16, 2018
@dajva dajva closed this Dec 16, 2018
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.

None yet

3 participants