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

Jump to bottom

vimPlugins.coc-prettier: Add override #72506

Merged
merged 4 commits into from Mar 15, 2020
Merged

vimPlugins.coc-prettier: Add override #72506

merged 4 commits into from Mar 15, 2020

Conversation

ersinakinci
Copy link
Contributor

@ersinakinci ersinakinci commented Nov 2, 2019 •
edited

Motivation for this change

This PR adds an override for vimPlugins.coc-prettier as a partial fix for #64560. Depends on #72501. Previously depended on #72501, which has now been squashed into this PR.

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 nix-review --run "nix-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 @rvolosatovs

@ersinakinci ersinakinci mentioned this pull request Nov 2, 2019
Closed
@ofborg ofborg bot added the 6.topic: vim label Nov 2, 2019
@jonringer
Copy link
Contributor

since the 2 commits are necessary to perform the fix, do you mind squashing them both with the commit message:

vimPlugins.coc-prettier: Add node dependency

@ersinakinci
Copy link
Contributor Author

@jonringer done!

@ersinakinci ersinakinci mentioned this pull request Nov 2, 2019
Closed
10 tasks
@jonringer
Copy link
Contributor

@GrahamcOfBorg build vimPlugins.coc-prettier

@jonringer
Copy link
Contributor

hmm, im just not sure about the several thousand node packages bump ... :(

@rvolosatovs
Copy link
Member

@jonringer I believe that's a side effect of node package packaging approach: https://github.com/NixOS/nixpkgs/blob/95dfbe2d63/doc/languages-frameworks/node.section.md#L37-L47

@ersinakinci
Copy link
Contributor Author

ersinakinci commented Nov 2, 2019 •
edited

@jonringer, what @rvolosatovs said. I also ran this by @adisbladis in the #nixos IRC channel and they confirmed that this is normal and even desirable.

I think there's a broader conversation to be had about plugin/package management policy as the "updates a million things" behavior is the standard workflow for plugins/packages in other Nixpkgs subsystems (e.g., Vim, Ruby). It's definitely surprising for people coming from outside of Nix and I'm not totally sold on it. But in the context of this particular PR, this is actually the blessed path.

@ersinakinci
Copy link
Contributor Author

Any progress on this?

jonringer
jonringer reviewed Nov 20, 2019
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.

Please also change the commit history so that it's not broken if at a particular commit (you reference nodePackages.coc-prettier before it was added)

the history should reflect:

nodePackages.coc-prettier: init
vimPlugins.overrides: add coc-prettier
neovim: use TMPDIR as home during initilizaiton

pkgs/development/node-packages/node-packages-v12.nix Outdated
@@ -2047,13 +2047,13 @@ let
sha512 = "w2VwSrBoHa5BsSyH+KxEqeQBAllHhccyMFVHtGtdMpF4W7IRWfZjFiQceJPChOeTsSDVUpER2T8FA93pr0L+QA==";
};
};
"temp-0.9.0" = {
"temp-0.9.1" = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is unrelated, you added a package to the v10 list, but an entry was updated in the v12 list

pkgs/misc/vim-plugins/overrides.nix Show resolved Hide resolved
pkgs/development/node-packages/node-packages-v10.json
@@ -16,6 +16,7 @@
, "browserify"
, "castnow"
, "clean-css"
, "coc-prettier"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't that cause a change in pkgs/development/node-packages/node-packages-v10.nix, instead it generates a new file, which seems weird to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as a reminder from November, I agree that this is weird, but apparently regenerating the entire node-packages-v10.nix and updating a ton of NPM packages in the process is the blessed path. I'm basing this off of 1) a conversation I had in the #nixos IRC channel and 2) the documentation.

pkgs/misc/vim-plugins/overrides.nix Outdated Show resolved Hide resolved
@felschr
Copy link
Member

felschr commented Jan 13, 2020

Any update on this?

@ersinakinci
Copy link
Contributor Author

Hey all, I got distracted from finishing this up. I'm going to incorporate @jonringer's suggestions and clean up this PR, hopefully we can get this merged soon.

@ersinakinci
Copy link
Contributor Author

@jonringer, I've cleaned up this PR and incorporated your suggestions from November. Let me know what you think.

@ersinakinci
Copy link
Contributor Author

Friendly ping 🙂.

If we can merge this, I have a bunch of other coc extensions that I want to merge, as well. I've successfully been using this approach for ~10 extensions on my local machine via overlays.

@felschr felschr mentioned this pull request Feb 24, 2020
Closed
10 tasks
@felschr felschr mentioned this pull request Mar 15, 2020
Merged
10 tasks
@adisbladis adisbladis merged commit 864be2e into NixOS:master Mar 15, 2020
@fnords fnords mentioned this pull request Mar 29, 2020
Closed
10 tasks
@zhaofengli zhaofengli mentioned this pull request Jun 30, 2021
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer left review comments

@rvolosatovs rvolosatovs rvolosatovs approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@ersinakinci @jonringer @rvolosatovs @felschr @adisbladis