-
Notifications
You must be signed in to change notification settings - Fork 769
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
[READY] Make python binary specifiable by the user #295
Conversation
I don't see anything wrong with this approach. :) Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. ycmd/completers/python/jedi_completer.py, line 75 [r1] (raw file): ycmd/completers/python/jedi_completer.py, line 78 [r1] (raw file): ycmd/completers/python/jedi_completer.py, line 91 [r1] (raw file): Comments from the review on Reviewable.io |
Ok, then I will try it. Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. ycmd/completers/python/jedi_completer.py, line 91 [r1] (raw file): Comments from the review on Reviewable.io |
97dacdc
to
959bfcb
Compare
Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions. ycmd/completers/python/jedi_completer.py, line 75 [r1] (raw file): ycmd/completers/python/jedi_completer.py, line 78 [r1] (raw file): Comments from the review on Reviewable.io |
Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. ycmd/completers/python/jedi_completer.py, line 38 [r2] (raw file): ycmd/completers/python/jedi_completer.py, line 91 [r2] (raw file): Comments from the review on Reviewable.io |
Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions. ycmd/completers/python/jedi_completer.py, line 38 [r2] (raw file): Comments from the review on Reviewable.io |
c10430c
to
37c7b65
Compare
Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions. ycmd/completers/python/jedi_completer.py, line 91 [r2] (raw file): Comments from the review on Reviewable.io |
Reviewed 1 of 2 files at r1, 1 of 1 files at r3. ycmd/completers/python/jedi_completer.py, line 91 [r1] (raw file): Comments from the review on Reviewable.io |
☔ The latest upstream changes (presumably #303) made this pull request unmergeable. Please resolve the merge conflicts. |
37c7b65
to
c6d2617
Compare
I've added the tests for the user defined python binary path. I will add the tests for A note: to test the user defined option I created the Review status: 1 of 8 files reviewed at latest revision, 1 unresolved discussion. ycmd/completers/python/jedi_completer.py, line 91 [r1] (raw file): Comments from the review on Reviewable.io |
Review status: 1 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed. ycmd/tests/handlers_test.py, line 54 [r4] (raw file): Comments from the review on Reviewable.io |
Reviewable is failing for me, so can't make line comments, but had 2 thoughts:
Reviewed 1 of 2 files at r1, 7 of 7 files at r4. Comments from the review on Reviewable.io |
Also. Csharp tests are failing :/ Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed. Comments from the review on Reviewable.io |
c6d2617
to
634ba3f
Compare
There were a test in Csharp subcommands and in clang get_completion which used
I was thinking about it myself, and you may be right 😕 I didn't want to mention JediHTTP since is an implementation detail but could be the most sensible solution.
You mean the value the user gives us which is not an existing one? Review status: 8 of 10 files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
Yeah, sorry to be clear, i've added a line comment now Reviewed 2 of 2 files at r5. ycmd/completers/python/jedi_completer.py, line 85 [r5] (raw file): Comments from the review on Reviewable.io |
Anyway, LGTM with those small comments. :) Review status: all files reviewed at latest revision, 2 unresolved discussions. Comments from the review on Reviewable.io |
Reviewed 2 of 2 files at r1. Comments from the review on Reviewable.io |
I agree that the "with" vs. "decorator" approach is a matter of taste. For the with self.UserOptions( user_option_one = 'one_value', user_option_two = 'two_value' ):
... But I think it doesn't make sense to make it accept multiple user_options until we actually need it. Review status: all files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
Agreed. No YAGNI :)
Me too, though I also prefer decorator (for the same reasons as @micbou), but in order to support that easily without pain, you have to require Review status: all files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
Reviewed 2 of 2 files at r6. Comments from the review on Reviewable.io |
Yes I remember and for me depends of the usage of it; for this particular case I prefer the Review status: all files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
Review status: all files reviewed at latest revision, 1 unresolved discussion. ycmd/completers/python/jedi_completer.py, line 85 [r5] (raw file): Comments from the review on Reviewable.io |
Yeh, this is totally fine :) Review status: all files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
is this [ready] now ? Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending. Comments from the review on Reviewable.io |
I wanted to add the tests for the Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending. Comments from the review on Reviewable.io |
a09b36e
to
cf9bbb3
Compare
I've pushed the tests for the @reinhrst if you some spare time to test this would be great! Basically there is a user option for specify which python should be used to spawn the JediHTTP server to get completion from. We still have the problem that you specified with https://github.com/reinhrst/jedi-working-directory-example but that was a pre-existing problem so I think it would be one of the next things to tackle. When this is merged I'm going to update the YCM docs. |
:LGTM: Reviewed 1 of 1 files at r7. Comments from the review on Reviewable.io |
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful. Comments from the review on Reviewable.io |
I tested I just added a (pedantic) comment but this is Reviewed 5 of 7 files at r4, 2 of 2 files at r5, 1 of 2 files at r6. ycmd/tests/python/user_defined_python_test.py, line 40 [r7] (raw file): Comments from the review on Reviewable.io |
cf9bbb3
to
5b48d5f
Compare
Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion. ycmd/tests/python/user_defined_python_test.py, line 40 [r7] (raw file): Comments from the review on Reviewable.io |
Woo! Python 3 in ycmd. @homu r+ Nice one! A lot went into getting here, so thanks @vheon. I think python 3 support is one of (if not, the) most requested feature Reviewed 1 of 1 files at r8. Comments from the review on Reviewable.io |
📌 Commit 5b48d5f has been approved by |
[READY] Make python binary specifiable by the user I'm starting this PR as WIP because it lacks tests and I wanted to ask for suggestion about that. What do you think would be the best approach? I was thinking about configuring the `user_option` and check how the `utils.SafePopen` was called but in general I'm not a fan of this kind of tests, so I'm open to suggestion 😃 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/295) <!-- Reviewable:end -->
Apparently in my fork one of the test failed due to the typescript |
that's been happening a bit recently - we're still not sure if it is a bug in the completer or in |
☀️ Test successful - status |
Very cool to see this working 👍 Would be nice to search the PATH for the executable. That way one could just use When testing (in a python3 virtualenv), it worked partly.
indeed only gives me the completions for python3. However
results in a crash. Didn't really have time to dive into it, but these are the logs (this is with master (d05bf55) of YouCompleteMe, master (bf256b2) or ycmd, all dependencies on the suggested version): ycmd stderr:
jedihttp stderr:
Python executable is |
We could think of passing the PATH env to the Popen; it should work then 👍
I don't like the crash, but from the logs it appears to be a jedi issue, am I wrong? maybe I need to use the dev branch instead of master? |
It seems to do that automatically but there are some issues (especially on Windows): http://stackoverflow.com/a/5659249/1207489 . In addition,
I just checked, indeed it crashes on master of jedi. @davidhalter , which branch/tag/sha makes most sense to use? See davidhalter/jedi#685 |
So is more complicated than I thought 😕 |
This may do the trick, e.g. making |
Just for reference, using the following 2 lines give a really nice experience
First makes sure the python is used from the current virtual env. Seconds adds the directory in which vim was started to the pythonpath, allowing completions from files and packages in that directory. |
I'm glad that we could finally improve (even if slightly) the python experience 😃
I will try to came up with a solution for those two problems.
|
Does
|
Don't know. I'm not suggesting my line to be a generic solution, just might help some people testing / adapting this to their own situation. I agree with @vheon that this should be solved in some generic way. |
Not natively. However, there is the |
I'm starting this PR as WIP because it lacks tests and I wanted to ask for suggestion about that. What do you think would be the best approach? I was thinking about configuring the
user_option
and check how theutils.SafePopen
was called but in general I'm not a fan of this kind of tests, so I'm open to suggestion 😃