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

nixos/pazi: init #60324

Closed
wants to merge 2 commits into from
Closed

nixos/pazi: init #60324

wants to merge 2 commits into from

Conversation

bbigras
Copy link
Contributor

@bbigras bbigras commented Apr 27, 2019

Motivation for this change

Shell integration.

@teto

Things done

I'm still running nixos-build but the module seems pretty simple. I'll update after testing.

I manually tested that eval works for bash and zsh. And pazi init fish | source for fish.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@bbigras
nixos/pazi: init
4cc097b 
mentioned in the NixOS#60303 PR
@bbigras bbigras requested a review from infinisil as a code owner April 27, 2019 14:39
@teto
Copy link
Member

teto commented Apr 27, 2019

I would prefer to see thing slike install -D misc/__khal $out/share/zsh/site-functions/__khal(as in the khal program). Calling the binary to generate the script as is done here is slower (?)! maybe you can save the completion in the package or upstream this. Or drop everything if this is too much work :)

@bbigras
Copy link
Contributor Author

bbigras commented Apr 27, 2019

Oh yeah calling pazi init every time the shell starts must waste time. I think I'll save the completion in the package.

@teto
Copy link
Member

teto commented Apr 27, 2019

yes get rid of the module, other programs provide completion without the module, some package paths are already looked for by the shell ($out/share/zsh/site-functions/ for zsh for instance).

@bbigras
Copy link
Contributor Author

bbigras commented Apr 27, 2019

some package paths are already looked for by the shell ($out/share/zsh/site-functions/ for zsh for instance)

I'm a bit confused about zsh. Is site-functions only for completion? Are aliases considered completion too? I think I saw that files related to completion should be named with an underscore prefix.

Not sure if I need #compdef pazi and #autoload.

@bbigras bbigras changed the title nixos/pazi: init [WIP] nixos/pazi: init Apr 29, 2019
@bbigras bbigras changed the title [WIP] nixos/pazi: init nixos/pazi: init May 7, 2019
@bbigras
Copy link
Contributor Author

bbigras commented May 7, 2019

I gave up on site-functions and bash-completion since I think they are used to load "on demand" autocompletion stuff. While in this case the shell files are used to set an alias and other stuff.

I changed nixos/modules/programs/pazi.nix to include the generated shell scripts. I don't know if I should or could do it in the modules.

I put them in ${pkgs.pazi}/share/. Not sure if it's ok.

I tested bash, zsh and fish and they seems to work fine.

@bbigras
Copy link
Contributor Author

bbigras commented May 10, 2019

@infinisil friendly reminder for the review. I can open a new PR if this one is too confusing.

infinisil
infinisil reviewed May 14, 2019
View reviewed changes
nixos/modules/programs/pazi.nix

programs.bash.interactiveShellInit = "source ${pkgs.pazi}/share/pazi.bash";
programs.fish.interactiveShellInit = "source ${pkgs.pazi}/share/pazi.fish";
programs.zsh.interactiveShellInit = "source ${pkgs.pazi}/share/pazi.zsh";
Copy link
Member

Choose a reason for hiding this comment

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

I'd much rather have some conventions for where to put stuff that should be auto-sourced. Then people could just do systemPackages = [ pkgs.pazi ] and their shell would load it because pazi.* appears in some /run/current-system/sw/share/* path. This would then also work for nix-env and on non-NixOS systems.

@bbigras
Copy link
Contributor Author

bbigras commented May 14, 2019

@infinisil I agree.

@bbigras bbigras closed this May 14, 2019
@bbigras bbigras deleted the pazi branch May 14, 2019 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bbigras @teto @infinisil