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

vim: add plugins #4499

Closed
wants to merge 1 commit into from
Closed

Conversation

jagajaga
Copy link
Member

Add some plugins by the tool I've created.
https://gist.github.com/3c7ba009ee6756e12978 (updated vim2nix)

@bjornfor
Copy link
Contributor

Why do you add path = "YouCompleteMe";? YouCompleteMe seems to work fine without it. (At least on my system.)

Git commit ids don't work well as a version numbers. Can you replace it with "commit date" or "commit date + git commit id" as version?

@ts468
Copy link
Contributor

ts468 commented Oct 12, 2014

@bjornfor, I have a function which maps over all vim plugins, and so I added the attribute "path" as it is available for the other plugins, but not for YouCompleteMe.

@jagajaga
Copy link
Member Author

@bjornfor added path because every plugin has it. And yes, that was @ts468's commit.
Git id's are well in other pkgs, why they are not well here?

@peti
Copy link
Member

peti commented Oct 12, 2014

@jagajaga, git ids are more or less random hashes that don't increment monotonously for new version, which breaks nix-env -u. Such hashes shouldn't be used as version numbers anywhere. If you are aware of any package that does this, please let me know because this is a bug that should be fixed.

@bjornfor
Copy link
Contributor

Not only do git hashes not increment like normal version numbers, they may also begin with a non-digit character, causing nix-env to not see it as a version number at all (instead it becomes a part of the package name, in the eyes of nix-env). When you try to install such a package it will cause a conflict with the older package. Really bad usability :-)

@bjornfor
Copy link
Contributor

@jagajaga: Ah, I didn't see the comment about tool. Thanks for the explanation.

Btw, I've been wanting to get rid of the "path" argument, or at least make it optional, because all plugins just install to a path made up of their name anyway. So currently "path" just duplicates plugin name.

@bjornfor
Copy link
Contributor

@jagajaga: If you want a bigger database of vim plugins, maybe https://github.com/MarcWeber/vim-addon-manager would be of interest?

@jagajaga
Copy link
Member Author

@bjornfor @peti Ok, I will do everything you've said. But please, help me to automate the sha256 getting.

@bjornfor
Copy link
Contributor

@jagajaga: Try "nix-prefetch-git" from the "nix-prefetch-scripts" package.

@jagajaga
Copy link
Member Author

@bjornfor done.

@bjornfor
Copy link
Contributor

@jagajaga: It seems some of the new plugins still use git hash as version.

I assume there are no release / git tags for these plugins? Since we don't package multiple variants (e.g. stable and bleeding edge), I don't think having "-git" in package name is needed. (And if it did, "git" should be part of the package attrname too.)

Speaking of package attrnames, they should have dashes instead of camelCase, to match package name. (Yes, there are offenders already in nixpkgs, but we're trying to clean up.)

Note that simpleDerivation does not add any prefix to plugin names. So that, for example, the "rust-..." thing being added here will be named "rust-..." in nix-env too. Until we add something like namePrefix ? "vimplugin" in simpleDerivation, plugins should add a prefix themself.

@jagajaga
Copy link
Member Author

@bjornfor it's ok now?

@bjornfor
Copy link
Contributor

@jagajaga: Oh, you changed existing stuff too? I didn't expect you to do that, but ok :-)

Can you do the cleanup in a separate commit, preserving old attributes for backward compatibility, like command_T = command-t; # added YYYY-MM-DD? And then "add plugins"?

(I don't feel strongly about the backward compatibility of vim plugin attributes, but it cost little to add them.)

I still see some upper/lower case mix in package attr + name. Please use lower-case everywhere.

@jagajaga
Copy link
Member Author

@bjornfor didn't get what you mean. Can you do the cleanup in a separate commit, preserving old attributes for backward compatibility, like command_T = command-t; # added YYYY-MM-DD? And then "add plugins"?

@bjornfor
Copy link
Contributor

@jagajaga: I mean that doing clean-up to existing plugins and adding new plugins are two logically separate changes; it would be natural to have one "cleanup" commit and one "add plugins" commit. Combining those two changes in a single commit named "add plugins" is wrong/bad, IMHO.

@jagajaga
Copy link
Member Author

@bjornfor that's going to be very hard (I have to redo everything). Indeed it's really nearly the same things (they are updated, no?). I can rename commit if it's ok.

@bjornfor
Copy link
Contributor

@jagajaga: I've done some vim-plugin clean-up / refactoring in #4580. If nobody objects, I'd like to get that merged first and then we can add more plugins on top of that.

@jagajaga
Copy link
Member Author

@bjornfor that's nice!
I am waiting for merging.

@bjornfor
Copy link
Contributor

@jagajaga: I've merged #4580. When you have the time, please rebase and force-push to update this PR.

@jagajaga
Copy link
Member Author

@bjornfor fuf. done :)
Please, can you look at #4569 ?

@jagajaga jagajaga force-pushed the vim_plugins_update branch 2 times, most recently from 7242009 to f3a24af Compare October 21, 2014 12:27
@jagajaga
Copy link
Member Author

Btw, it's strange behavior of travis CI, because on my local machine it's all has built succesfully.

@bjornfor
Copy link
Contributor

@jagajaga: Thanks. Looks good (and builds fine here too). Pushed to master in commit 454590f.

@bjornfor bjornfor closed this Oct 21, 2014
jagajaga added a commit that referenced this pull request Feb 7, 2015
Using date instead of rev
See #4499 (comment) for more info why.
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.

4 participants