-
Notifications
You must be signed in to change notification settings - Fork 770
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] Let ycmd check rustup configured path as the last means to finding rust src path #785
Conversation
A testing approach I'd take would be to mock out the troubling parts. For instance, you can mock out Review status: 0 of 1 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
Codecov Report
@@ Coverage Diff @@
## master #785 +/- ##
==========================================
+ Coverage 94.83% 94.83% +<.01%
==========================================
Files 79 79
Lines 5398 5406 +8
Branches 169 169
==========================================
+ Hits 5119 5127 +8
Misses 231 231
Partials 48 48 |
Before I push the tests, one question. Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks broke. Comments from Reviewable |
I don't think I understand the question. Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks broke. Comments from Reviewable |
Do we have something like that? Do we have a valid rust src on continous integration bots? I just think this needs a test where we show that semantic completion is working without any setup required. I hope I was clearer now. Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks broke. Comments from Reviewable |
What I would do is create the Reviewed 1 of 1 files at r1. ycmd/completers/rust/rust_completer.py, line 142 at r1 (raw file):
I would assign the result of this function to a variable and then use this variable in the ycmd/completers/rust/rust_completer.py, line 143 at r1 (raw file):
Indentation should be two spaces. ycmd/completers/rust/rust_completer.py, line 145 at r1 (raw file):
ycmd/completers/rust/rust_completer.py, line 150 at r1 (raw file):
This should be rewritten to Comments from Reviewable |
Yup, pretty much what @micbou said. :) Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. ycmd/completers/rust/rust_completer.py, line 145 at r1 (raw file): Previously, micbou wrote…
Can we wrap that somehow or fix ycmd/completers/rust/rust_completer.py, line 150 at r1 (raw file): Previously, micbou wrote…
Good catch! Comments from Reviewable |
88a4216
to
79a895b
Compare
I must me doing something stupid, but I can't figure out what's wrong. The tests I added all say:
Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions. ycmd/completers/rust/rust_completer.py, line 142 at r1 (raw file): Previously, micbou wrote…
Done. ycmd/completers/rust/rust_completer.py, line 143 at r1 (raw file): Previously, micbou wrote…
Done. ycmd/completers/rust/rust_completer.py, line 145 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. ycmd/completers/rust/rust_completer.py, line 150 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. Comments from Reviewable |
That's because you are patching the Reviewed 3 of 3 files at r2. ycmd/completers/rust/rust_completer.py, line 145 at r1 (raw file): Previously, bstaletic wrote…
Found out that it works on Windows when we also open a pipe to utils.SafePopen( [ 'rustc', '--print', 'sysroot' ], stdin_windows = subprocess.PIPE, stdout = subprocess.PIPE, stderr = subprocess.PIPE ) so we could do that. ycmd/completers/rust/rust_completer.py, line 76 at r2 (raw file):
I would use ycmd/tests/rust/get_completions_test.py, line 151 at r2 (raw file):
Why don't we use ycmd/tests/rust/get_completions_test.py, line 163 at r2 (raw file):
Same comment. Comments from Reviewable |
I've added
The log is then followed by a traceback. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. ycmd/completers/rust/rust_completer.py, line 76 at r2 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 151 at r2 (raw file): Previously, micbou wrote…
My bad. Done. ycmd/tests/rust/get_completions_test.py, line 163 at r2 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
79a895b
to
8505947
Compare
Reviewed 1 of 1 files at r3. ycmd/tests/rust/get_completions_test.py, line 163 at r2 (raw file): Previously, bstaletic (Boris Staletic) wrote…
Why no ycmd/tests/rust/get_completions_test.py, line 166 at r2 (raw file):
See the Rust ycmd/tests/rust/get_completions_test.py, line 139 at r3 (raw file):
There are two issues in this test. First,
Second, we don't raise the ycmd/tests/rust/get_completions_test.py, line 152 at r3 (raw file):
Same issue as above: we don't raise the ycmd/tests/rust/get_completions_test.py, line 169 at r3 (raw file):
Comments from Reviewable |
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed. ycmd/completers/rust/rust_completer.py, line 145 at r1 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 163 at r2 (raw file): Previously, micbou wrote…
No good reason, fixed. ycmd/tests/rust/get_completions_test.py, line 166 at r2 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 139 at r3 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 152 at r3 (raw file):
After taking another look at thte code, I think you're right, so I removed the test. ycmd/tests/rust/get_completions_test.py, line 169 at r3 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
3bd53eb
to
25963cc
Compare
Reviewed 2 of 2 files at r4. ycmd/tests/rust/get_completions_test.py, line 169 at r3 (raw file): Previously, bstaletic (Boris Staletic) wrote…
PathToTestFile( 'rustup-toolchain', 'lib', 'rustlib', 'src', 'rust', 'src' ) ycmd/tests/rust/get_completions_test.py, line 142 at r4 (raw file):
I don't think we want to complete in the Rust standard library but rather in ycmd/tests/rust/get_completions_test.py, line 165 at r4 (raw file):
Comments from Reviewable |
25963cc
to
80789a7
Compare
Review status: 2 of 3 files reviewed at latest revision, 6 unresolved discussions. ycmd/tests/rust/get_completions_test.py, line 169 at r3 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 142 at r4 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 165 at r4 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
80789a7
to
cf7a1dd
Compare
I fixed the Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions. Comments from Reviewable |
Reviewed 1 of 1 files at r5, 1 of 1 files at r6. ycmd/completers/rust/rust_completer.py, line 76 at r2 (raw file): Previously, bstaletic (Boris Staletic) wrote…
I don't see the change. ycmd/tests/rust/get_completions_test.py, line 58 at r6 (raw file):
ycmd/tests/rust/get_completions_test.py, line 151 at r6 (raw file):
You need Comments from Reviewable |
cf7a1dd
to
3e09cf5
Compare
Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions. ycmd/completers/rust/rust_completer.py, line 76 at r2 (raw file): Previously, micbou wrote…
My bad. Now it is done. ycmd/tests/rust/get_completions_test.py, line 58 at r6 (raw file): Previously, micbou wrote…
Done. ycmd/tests/rust/get_completions_test.py, line 151 at r6 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
Reviewed 1 of 2 files at r7. ycmd/completers/rust/rust_completer.py, line 76 at r2 (raw file): Previously, bstaletic (Boris Staletic) wrote…
Should be ycmd/tests/rust/get_completions_test.py, line 80 at r7 (raw file):
This test should be patched like Comments from Reviewable |
Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions. ycmd/tests/rust/get_completions_test.py, line 61 at r7 (raw file):
Need to add Comments from Reviewable |
3e09cf5
to
fcd62aa
Compare
@micbou Thanks for the help. This should be READY, if no one has any other changes to request. Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions. Comments from Reviewable |
Reviewed 1 of 1 files at r11. Comments from Reviewable |
Review status: all files reviewed at latest revision, 4 unresolved discussions. ycmd/tests/rust/get_completions_test.py, line 59 at r11 (raw file):
strictly we should use though i guess it doesn't really matter in this case. Comments from Reviewable |
Reviewed 1 of 3 files at r2, 1 of 2 files at r8, 1 of 1 files at r11. Comments from Reviewable |
7b40de9
to
1609926
Compare
Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions. ycmd/tests/rust/get_completions_test.py, line 59 at r11 (raw file): Previously, puremourning (Ben Jackson) wrote…
Heh, I though I fixed that everywhere. Comments from Reviewable |
Reviewed 1 of 1 files at r12. ycmd/tests/rust/get_completions_test.py, line 59 at r11 (raw file): Previously, bstaletic (Boris Staletic) wrote…
Why don't we just set Comments from Reviewable |
1609926
to
8316a86
Compare
Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful. ycmd/tests/rust/get_completions_test.py, line 59 at r11 (raw file): Previously, micbou wrote…
Alright, changed. Comments from Reviewable |
Reviewed 1 of 1 files at r13. ycmd/tests/rust/get_completions_test.py, line 82 at r13 (raw file):
same here Comments from Reviewable |
8316a86
to
a2ad08b
Compare
Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions. ycmd/tests/rust/get_completions_test.py, line 82 at r13 (raw file): Previously, puremourning (Ben Jackson) wrote…
Done. Comments from Reviewable |
This looks good to go! @zzbot r=micbou Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion. Comments from Reviewable |
📌 Commit a2ad08b has been approved by |
[READY] Let ycmd check rustup configured path as the last means to finding rust src path In ycm-core/YouCompleteMe#2379 there was talk about enabling ycmd to check rust src path provided by rustup. This allows users to have rust completion with zero ycmd configuration. Tests are not included because tests themselves should behave differently based on how rustup was used and I'm not sure how to test that. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/785) <!-- Reviewable:end -->
💔 Test failed - status-appveyor |
What happened to appveyor? @zzbot retry |
[READY] Let ycmd check rustup configured path as the last means to finding rust src path In ycm-core/YouCompleteMe#2379 there was talk about enabling ycmd to check rust src path provided by rustup. This allows users to have rust completion with zero ycmd configuration. Tests are not included because tests themselves should behave differently based on how rustup was used and I'm not sure how to test that. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/785) <!-- Reviewable:end -->
☀️ Test successful - status-appveyor, status-travis |
[READY] Fix Rust debug info test when rustup is installed Since PR #785, the Rust debug info test fails if the Rust sources have been downloaded with rustup on the machine running the tests. We should check that the Rust sources value is either `None` or a string. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/850) <!-- Reviewable:end -->
[READY] Update ycmd Include the following changes: - PR ycm-core/ycmd#785: automatically find Rust sources through rustup; - PR ycm-core/ycmd#835: do not return canonical type if identical to type in C-family languages; - PR ycm-core/ycmd#837: improve support of system Boost and system libclang on Gentoo; - PR ycm-core/ycmd#840: improve Red Hat and CentOS detection; - PR ycm-core/ycmd#842: consider header file entries in compilation database; - PR ycm-core/ycmd#843: improve completion of include statements in C-family languages; - PR ycm-core/ycmd#851: rename completer options in installation script; - PR ycm-core/ycmd#855: only include one macOS toolchain. Update documentation according to PRs ycm-core/ycmd#785 and ycm-core/ycmd#851. Close #2379. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2804) <!-- Reviewable:end -->
In ycm-core/YouCompleteMe#2379 there was talk about enabling ycmd to check rust src path provided by rustup.
This allows users to have rust completion with zero ycmd configuration.
Tests are not included because tests themselves should behave differently based on how rustup was used and I'm not sure how to test that.
This change is