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 the Rust programming language #1888

Merged
merged 4 commits into from
Jan 9, 2016

Conversation

jwilm
Copy link
Contributor

@jwilm jwilm commented Jan 7, 2016

Support for the Rust programming language is added using ycmd's racerd completer. This PR contains documentation for using the Rust completer and an update to the ycmd submodule.

TODO

Review on Reviewable

@micbou
Copy link
Collaborator

micbou commented Jan 7, 2016

Reviewed 1 of 3 files at r1.
Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


README.md, line 351 [r1] (raw file):
Could you remove the ./ in the ./install.py occurrences for the Windows installation? There is no such thing in cmd.exe.


README.md, line 897 [r1] (raw file):
Comments in viml are starting with ".


README.md, line 898 [r1] (raw file):
Missing single quote at the end of line.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Jan 7, 2016

Reviewed 1 of 1 files at r2.
Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
Question for everyone which will be relevant for python in the near future: do we want to say racerd-based or racer-based ... using racerd? I mean the completion is engine that is doing the "real" work is racer and we use it through racerd. To be clear, I don't want to discredit @jwilm work, is just to inform the user what we're actually using, thats it. We actually already have a case like this with C# where we say: "an OmniSharp-based completion engine ..." but we don't talk about OmniSharpServer which is what we use at the moment.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Jan 7, 2016

Reviewed 2 of 3 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from the review on Reviewable.io

@micbou
Copy link
Collaborator

micbou commented Jan 7, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
I agree. It should be racer even if it is obvious that racerd is related to racer.


Comments from the review on Reviewable.io

@jwilm
Copy link
Contributor Author

jwilm commented Jan 7, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 898 [r1] (raw file):
Oh wow, that was sloppy. Appreciate your attention to detail :).


Comments from the review on Reviewable.io

@puremourning
Copy link
Member

Review status: all files reviewed at latest revision, 5 unresolved discussions.


README.md, line 173 [r2] (raw file):
This needs updating. And the others.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Valloric commented Jan 8, 2016

Review status: all files reviewed at latest revision, 7 unresolved discussions.


README.md, line 51 [r2] (raw file):
I don't particularly care which we use because IMO it really doesn't matter, in general and especially in this case because "racerd" contains "racer".

Joe can pick whichever he prefers here. I'd go with racerd, but again, I don't really care.


README.md, line 892 [r2] (raw file):
Let's avoid any confusion by stating "the rust source code" and not just "the rust source."

Also, "a configuration is needed" sounds a bit weird. How about:

You also need to set the following option [...]


README.md, line 1130 [r2] (raw file):
I don't think this distinction is necessary. We don't do it for the other languages and for anyone who knows the difference between declaration and definition will know they lead to the same thing in Rust. If they don't know the difference, the explanation won't help much.

For Rust, people will just use GoTo anyway.


README.md, line 1154 [r2] (raw file):
Same as above, we can drop this.


Comments from the review on Reviewable.io

@jwilm
Copy link
Contributor Author

jwilm commented Jan 8, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
I'm going to favor racer over racerd in the docs for reason of convention.

Whether <lang-completer>-server or <lang-completer> should be referenced seems like a separate discussion that, if change is decided on, should address all semantic providers in the README together (ie not in this PR). The servers feel like an implementation detail that aren't important to 99% of README consumers. New YCM contributors who need to know that there are things like JediHTTP or racerd will find out as soon as they open the ycmd source. That said, it's always nice to have more visibility to personal projects, and it would be nice to acknowledge these efforts some how.


README.md, line 173 [r2] (raw file):
Done.


README.md, line 892 [r2] (raw file):
The a was not meant to be there in this case, but I still like your version better :). Done.


README.md, line 1130 [r2] (raw file):
Agreed. Done.


Comments from the review on Reviewable.io

@jwilm
Copy link
Contributor Author

jwilm commented Jan 8, 2016

Should I document the g:ycm_racerd_binary_path user option here? It's definitely a ycmd/racerd dev feature, and this is more of the "consumer" facing document.


Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Jan 8, 2016

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):

That said, it's always nice to have more visibility to personal projects, and it would be nice to acknowledge these efforts some how.

Is for that reason that initially I proposed:

- a [racer][]-based completion engine for Rust, using [racerd][]

So we would both inform the user that the engine used is racerd and give credit to the author of the interface that we use.


Comments from the review on Reviewable.io

@jwilm
Copy link
Contributor Author

jwilm commented Jan 8, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
That's a nice compromise. If I am to update the racer line in this PR, the others should probably be updated too. There's JediHTTP, OmniSharpServer, and racerd. Are there others?


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Jan 8, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
For JediHTTP I have to write some more documentation since is missing the RestartServer subcommand and I'm about to add another feature which will enable python3 completion which has to be documented as well so I could do it all later but if you want to cover this particular thing that is ok for me 👍


Comments from the review on Reviewable.io

@mispencer
Copy link
Contributor

README.md, line 51 [r2] (raw file):
If you are going to rewrite it, OmnisharpServer is actually a HTTP wrapper around NRefactory.


Comments from the review on Reviewable.io

@jwilm
Copy link
Contributor Author

jwilm commented Jan 9, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
If nobody protests, I'd like to just leave it as-is for now 😄.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member

It is worth documenting it I think because we have no other place to do so conveniently, and otherwise it's just magic. But it shouldn't block merging in my opinion :)

Remember some part of vim ethos is all about good and complete docs. :)

With that LGTM.


Reviewed 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
I don't protest. Let's get rust support merged. Docs are easy to change. And you worked your but off for this so kudos.


Comments from the review on Reviewable.io

@jwilm
Copy link
Contributor Author

jwilm commented Jan 9, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


README.md, line 51 [r2] (raw file):
Haha thanks 😄. Definitely ready to get this out the door!


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Valloric commented Jan 9, 2016

:lgtm: Let's ship this thing! :)


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Valloric commented Jan 9, 2016

@homu r+


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@homu
Copy link
Contributor

homu commented Jan 9, 2016

📌 Commit 210db41 has been approved by Valloric

@homu
Copy link
Contributor

homu commented Jan 9, 2016

⚡ Test exempted - status

@homu homu merged commit 210db41 into ycm-core:master Jan 9, 2016
homu added a commit that referenced this pull request Jan 9, 2016
Add support for the Rust programming language

Support for the Rust programming language is added using ycmd's racerd completer. This PR contains documentation for using the Rust completer and an update to the ycmd submodule.

## TODO
- [x] Update ycmd submodule once ycm-core/ycmd#268 lands

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/1888)
<!-- Reviewable:end -->
@dbrgn
Copy link

dbrgn commented Jan 9, 2016

Wohoo, this is awesome! Thanks a lot to all involved! :)

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.

8 participants