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.

Sign up for GitHub

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

Jump to bottom

vim-plugins: turn filetype and syntax before sourcing the plugins #77143

Merged
merged 5 commits into from Jan 7, 2020
Merged

vim-plugins: turn filetype and syntax before sourcing the plugins #77143

merged 5 commits into from Jan 7, 2020

Conversation

kalbasit
Copy link
Member

@kalbasit kalbasit commented Jan 6, 2020

Motivation for this change

Currently, all the plugins are loaded after all the plugins have already been loaded. This could break some plugins, for instance, vim-terraform assumes the filetype detection has already been enabled.

This commit follows VAM's recommendation and enables filetype and syntax before sourcing any of the plugins.

This was discovered after a follow up with vim-terraform's maintainer on an upstream pull request: hashivim/vim-terraform#133

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @timokau @gloaming

@kalbasit kalbasit mentioned this pull request Jan 6, 2020
Closed
@kalbasit
Copy link
Member Author

kalbasit commented Jan 6, 2020

Please backport this to 19.09.

@jonringer jonringer requested a review from Ma27 January 6, 2020 22:57
@dimbleby
Copy link

dimbleby commented Jan 6, 2020

It's possible that you're going to get into a tangle here: some plugin managers have the opposite requirement. Eg pathogen asks you to turn filetype on after doing its thing and vundle explicitly asks you to set filetype off first...

Good luck!

@kalbasit
Copy link
Member Author

kalbasit commented Jan 6, 2020

It's possible that you're going to get into a tangle here: some plugin managers have the opposite requirement. Eg pathogen asks you to turn filetype on after doing its thing and vundle explicitly asks you to set filetype off first...

Good luck!

Ah, thank you for pointing that out @dimbleby. I removed vundle and neobundle since they're not actually implemented and will probably include the settings directly with the plugin implementation for this to work correctly.

@kalbasit kalbasit force-pushed the nixpkgs_enable-syn-ft-before-plugins branch from 3131b33 to dba76a1 Compare January 6, 2020 23:33
@kalbasit
vim-plugins: honor the plugin manager re filetype and syntax
f18b391 
Currently, all the filetype and syntax are enabled *after* all the plugins has
already been loaded. Whilst this is the case for Pathogen, it's not
recommended when using VAM.

This commit applies the recommendation for:
- VAM[0]: The filetype and syntax are enabled *before* the plugins are loaded.
- Pathogen[1]: The filetype and syntax are enabled *after* the plugins are loaded.
- Plug[2]: The filetype and syntax are automatically enabled.

[0]: https://github.com/MarcWeber/vim-addon-manager/tree/d9e865f3c2de5d9b7eabbc976f606cf1b89e29ea#recommended-setup
[1]: https://github.com/tpope/vim-pathogen/blob/a553410f1bdb9000fbc764069f3a5ec3454a02bc/README.markdown#runtime-path-manipulation
[2]: https://github.com/junegunn/vim-plug/blob/2f5f74e5e67f657e9fdac54891a76721bcd3ead3/README.md#usage
@kalbasit kalbasit force-pushed the nixpkgs_enable-syn-ft-before-plugins branch from dba76a1 to f18b391 Compare January 6, 2020 23:45
@kalbasit
Copy link
Member Author

kalbasit commented Jan 6, 2020

I tested the following implementation with the vim-terraform plugin since it's the only one that falls on me if filetype was not enabled properly at the right time.

# Pathogen
nix-build -I nixpkgs=. -E 'with import <nixpkgs> {}; neovim.override { configure = { pathogen.pluginNames = [ "vim-terraform" ]; }; }' --show-trace

# VAM
nix-build -I nixpkgs=. -E 'with import <nixpkgs> {}; neovim.override { configure = { vam.pluginDictionaries = [ "vim-terraform" ]; }; }' --show-trace

All the above-compiled vim were then tested by opening a Terraform file, I see no errors and the filetype is set correctly to terraform instead of tf.

I was not able to verify that vim-terraform is able to load correctly with the native plugins (I'm not sure how that even works), so I left that implementation unchanged by adding the filetype indent plugin on | syn on at the end of the implementation. I tried with the following command

nix-build -I nixpkgs=. -E 'with import <nixpkgs> {}; neovim.override { configure = { packages.myVimPackage = { start = [ "vim-terraform" ]; }; }; }' --show-trace

@kalbasit kalbasit mentioned this pull request Jan 6, 2020
Merged
@timokau
Copy link
Member

timokau commented Jan 7, 2020

Didn't you open a similar PR already? This all seems very familiar to me.

Unfortunately I'm pretty short on time currently, so I can't test this extensively. I'm not entirely sure if turning both of these on is the right thing to do, but I'll let you and the other people interested in vim decide here.

@kalbasit
Copy link
Member Author

kalbasit commented Jan 7, 2020

Yes, indeed.

In #66536, we fixed the manifest generation but the runtime was still broken. This PR reverts #66536 and just fixes the root cause of us not respecting the recommend usage of the plugins we support, Pathogen, Vam and Plug.

jonringer
jonringer approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

[2 built, 1 copied (32.4 MiB), 6.8 MiB DL]
https://github.com/NixOS/nixpkgs/pull/77143
1 package built:
vimPlugins.vim-terraform

@jonringer
Copy link
Contributor

@GrahamcOfBorg build vimPlugins.vim-terraform

@jonringer jonringer merged commit 8dccb59 into NixOS:master Jan 7, 2020
@kalbasit kalbasit deleted the nixpkgs_enable-syn-ft-before-plugins branch January 7, 2020 21:24
@kalbasit kalbasit mentioned this pull request May 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 approved these changes

@jonringer jonringer jonringer approved these changes

@timokau timokau Awaiting requested review from timokau

@Mic92 Mic92 Awaiting requested review from Mic92

@gloaming gloaming Awaiting requested review from gloaming

Assignees
No one assigned
Labels
6.topic: vim 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kalbasit @dimbleby @timokau @jonringer @Ma27