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

vimPlugins: cleanup overrides #119356

Merged
merged 2 commits into from Apr 19, 2021
Merged

Conversation

malob
Copy link
Member

@malob malob commented Apr 13, 2021

Motivation for this change

I went to add some overrides, and noticed that the file might benefit from some cleanup. As far as I could tell, there wasn't a consistent policy used for ordering/organizing the overrides, and the arguments. For the former, I sorted them alphabetically. For the latter, I sorted them and organized them into what I think are pretty intuitive groups.

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.

, llvmPackages # clang_complete, clighter8
, meson # meson
, nim # fruzzy
, nodePackages # coc-*
Copy link
Member

Choose a reason for hiding this comment

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

that goes to languages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good old edge cases :p

I think both would be reasonable. I put this here, since it's being used exclusively to get the source for all the coc plugins. I'm inclined to leave it here, but I'm happy to move it if you still think is should go in the languages section.

}:

self: super: {

vim2nix = buildVimPluginFrom2Nix {
Copy link
Member

Choose a reason for hiding this comment

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

I am not feeling like proof reading all of this right now to spot any mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, thanks for the quick feedback on the arguments.

FWIW, all the changes from here down are just me sorting the top-level attributes alphabetically (case-insensitive). So I don't think there's much to proofread.

@malob malob force-pushed the cleanup-vimplugin-overrides branch from 4b4ec25 to 52885fc Compare April 13, 2021 19:42
@malob
Copy link
Member Author

malob commented Apr 13, 2021

When making the other changes I noticed that the use of self and super doesn't seem to be consistent when specifying the dependencies attribute. For example, some plugins listed in dependencies that have their own overrides are referenced using self, but more often than not super is used. I would have thought we'd always want to use self when adding an overridden plugin as a dependency of another plugin. If that's right, and I'm not confused about something here, I'd expect that we'd want to default to using self in these situations, so that dependencies will always use the overridden version of a plugin if it exists.

If that's correct, then as part of this cleanup, I'd be happy to go through any overrides that specify dependencies and change self to super unless there's an obvious reason the super should be used in any individual case.

@malob malob force-pushed the cleanup-vimplugin-overrides branch from 52885fc to c486c18 Compare April 19, 2021 18:22
@malob
Copy link
Member Author

malob commented Apr 19, 2021

(Rebased on master to fix merge conflict)

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I guess this is fine since it is zero rebuilds for actual vimPackages. @jonringer

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.

LGTM

Result of nixpkgs-review pr 119356 run on x86_64-linux 1

1 package built:
  • spacevim

@jonringer jonringer merged commit 78c9b87 into NixOS:master Apr 19, 2021
@malob malob deleted the cleanup-vimplugin-overrides branch April 19, 2021 21:57
@malob
Copy link
Member Author

malob commented Apr 19, 2021

Thanks! @jonringer, any interest in me tackling the super/self issue I mentioned here: #119356 (comment)

@jonringer
Copy link
Contributor

I would have thought we'd always want to use self when adding an overridden plugin as a dependency of another plugin.

self will refer to the package set with all overlays applied, and super should be the package set before your overlay is applied. If we do need to reference overriden plugins, then yes, they should reference self. So I think it would be worthwhile :)

@malob
Copy link
Member Author

malob commented Apr 21, 2021

Sweet, done here: #119977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants