-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
yazi: clean up wrapper, add options and format #321076
Conversation
Result of 1 package built:
|
I don't think we should add exiftool to the wrapper. Unlike other tools which are used to provide yazi functions in rust or lua codes, the only usage of exiftool is in the default opener config. And I don't think we should add tools only in config to the wrapper, because config can and usually be changed. |
@XYenon As a new user, I expect things to work out of the box without me having to configure anything. Also, looking at the history of that config file, That being said, if you still don't want to add it, how about adding an That way, users will easily be able to simply add the tools they need to the wrapper. |
What's the benefit to add packages to the wrapper instead of globally? |
@linsui Well, the main benefit is making sure those packages are installed/deleted with the program and that it functions as it should out of the box. For example, say that I want to try yazi but Therefore, by adding the tools we need to the wrapper, we're effectively coupling them to the main program. |
mpv, mediainfo, and even vscode are in the default openers, I don't think exiftool is special. Add a |
|
Tested the change with: $ nix-build -E 'with import ./. {}; (yazi.override { extraPackages = [ exiftool ]; })' And everything works correctly. |
Maybe we should unify those options. E.g.
|
How about putting file in optionalDeps and removing withRuntimeDeps? |
According to the yazi docs, file is not optional. Not sure about |
Without file yazi can still run and browse files though previewer doesn't work. I feel that it doesn't worth adding withRuntimeDeps only for that. withRuntimeDeps becomes useless anyway. |
I don't mind removing |
- Remove redundant `withX` options - Add `optionalDeps`: packages that provide additional features - Add `extraPackages`: user-defined packages - Format with nixfmt
Does it make sense to add Also, are these changes considered a breaking API change and thus can't be backported to the stable branch? |
It can also cause files to be unable to open - I think being able to open files is a baseline for a file manager, so it's worth mentioning here. |
Sounds good to me. |
Description of changes
Add exiftool, which is needed for showing EXIF data when interactively opening images withO
withPackage
optionsoptionalDeps
for packages that provide additional featuresextraPackages
for user-defined packagesThings done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.