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

Import rust files #1356

Closed
wants to merge 1 commit into from
Closed

Import rust files #1356

wants to merge 1 commit into from

Conversation

steveklabnik
Copy link

This is my first contribution to vim, so please bear with me as I learn the ropes.

This patch imports rust-lang/rust.vim@732b5fc into vim itself. I've built vim and confirmed that at least the syntax highlighting is working; frankly, I don't use many of its other features myself.

I am not an expert on vim plugins; some of the files included in rust.vim don't have a directory here in vim itself, like after/syntax. I am not sure if they're needed when not run as a plugin.

Any and all feedback welcome; I'm happy to update with any requirements. Thanks!

Addresses #1354

@steveklabnik
Copy link
Author

Oh, one thing I forgot:

When you do, please mention who you are, the license under which this is developed and that you give us allowance to distribute it with Vim.

I am on the Rust core team, and am one of the maintainers of rust.vim. rust.vim is distributed under the terms of both the MIT license and the Apache License (Version 2.0), at your option. As such, you don't need me to allow it, but I allow it regardless 😄

@ZyX-I
Copy link

ZyX-I commented Jan 5, 2017

If I am not mistaking, rust.txt should be ft_rust.txt with tags looking like ft-rust…, possibly with ft-rust-plugin, ft-rust-syntax and ft-rust-indent tags.

There are lots of examples of this convention, though e.g. ft_sql.txt does not follow it; other tags are normally present in syntax.txt (ft-…-syntax; though ft-ada-syntax is in ft_ada.txt), ), insert.txt (ft-…-omni, except for ft-ada-omni), indent.txt (ft-…-indent, except for ft-ada-indent), filetype.txt (ft-…-plugin, except for ft-ada-plugin): as you see, ft_ada.txt was allowed to contain them all instead of scattering documentation over the files. Compilers are described in quickfix.txt with tag like compiler-rust, but I do not know exceptions here.

@chrisbra
Copy link
Member

chrisbra commented Jan 5, 2017

this is for #1354
Thanks for that. If I am not mistaken, there are some functions, that seem not needed, e.g. autoload/rust.vim autoload/rustfmt.vim Or some files are missing. In particular I am missing the command definitions for those :RustRun , :RustExpand RustEmitIr RustEmitAsm and RustFmt and RustFmtRange command. Ah, I think, those commands are defined in ftplugin/rust.vim which is not included in this PR.

Also plugin/rust.vim only seems to be about syntastic, so we can probably skip that.

syntax/rust.vim and indent/rust.vim look fine

Also you might want to mention in those files, that they are maintained at https://github.com/rust-lang/rust.vim so that people having issues can direct them directly to the upstream repository.

@brammool
Copy link
Contributor

brammool commented Jan 6, 2017 via email

@steveklabnik
Copy link
Author

steveklabnik commented Jan 7, 2017

Thanks @chrisbra and @brammool ! I've pushed a second commit; please let me know if you want me to squash them.

@chrisbra,

Also plugin/rust.vim only seems to be about syntastic, so we can probably skip that.

Ah yes, we probably have some extra stuff that's not needed. I can remove them. syntastic has been causing us headaches anyway... I'll push a commit with this.

If I am not mistaken, there are some functions, that seem not needed, e.g. autoload/rust.vim autoload/rustfmt.vim Or some files are missing. In particular I am missing the command definitions for those :RustRun , :RustExpand RustEmitIr RustEmitAsm and RustFmt and RustFmtRange command.

Ah, I think, those commands are defined in ftplugin/rust.vim which is not included in this PR.

I've pushed a second commit that added ftplugin, don't know how I missed it the first time. Thanks!

@brammool
Copy link
Contributor

A few more remarks.

Please remove loading the matchit plugin, that is a user preference unrelated to rust.

Please use a tabstop of 8. And adjust the modeline for that.

The header for runtime/ftplugin/rust.vim says it's a syntax file.

As mentioned, please rename the doc file to ft_rust.vim

The ftplugin file is missing the link to the github project.

@steveklabnik
Copy link
Author

A quick update on this PR: the last few weeks have been very busy for me, but I intend to address @brammool 's comments no later than next week. Sorry about taking so long here!

From
rust-lang/rust.vim@732b5fc

Some modifications as per vim#1356 that will
be folded back upstream.
@steveklabnik
Copy link
Author

I believe I have now addressed all the review comments. Thanks again for your patience here.

@rhysd
Copy link
Contributor

rhysd commented Mar 8, 2017

Any updates on this? I think this would be a great contribution.

@steveklabnik
Copy link
Author

Done. Your build exited with 0.

Looks like this Travis failure was spurious, so I didn't say anything. Happy to do whatever to get this merged.

@chrisbra
Copy link
Member

included as of 3c2881d

@chrisbra chrisbra closed this Mar 22, 2017
@steveklabnik
Copy link
Author

Thank you so much!

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

5 participants