ranger: add imagePreviewSupport option and make previews work out of the box #26453
+17
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joachifm
reviewed
Jun 9, 2017
| @@ -1,4 +1,7 @@ | |||
| { stdenv, fetchurl, pythonPackages, w3m, file, less }: | |||
| { stdenv, fetchurl, pythonPackages, file, less | |||
| , imagePreviewSupport ? true, w3m ? null}: | |||
There was a problem hiding this comment.
I wonder what the purpose of defaulting w3m to null is? As far as I can tell, the expression fails to evaluate if w3m = null is in fact passed here (provided imagePreviewSupport = true). That'd require user intervention of course, but still seems odd to default to an invalid combination of parameter values.
|
The purpose is to show that `w3m` is in fact optional if you disable
image previews. A lot of packages do the same. I was too lazy to add an
assert because the build time is tiny anyway.
Since there seems to be no standard which "pattern" to use
{ optionSupport ? ..., package ? null }
assert optionSupport -> package != null;
... and be sure to not use the package when optionSupport is false ..
and then evaluate as .override { optionSupport = false; }
... or don't ...
and then evaluate as .override { optionSupport = false; package = null; }
This pattern requires the least amount of knowledge on the user
part. You callPackages without some of the optional packages and it
will work. I prefer this.
or
{ optionSupport ? ..., package }
and then
... and be sure to not use the package when optionSupport is false ..
and then evaluate as .override { optionSupport = false; }
... or don't ...
and then evaluate as .override { optionSupport = false; package = null; }
Okay in nixpkgs context, but can't be evaluated with callPackages
without some of the optional packages.
or
{ package ? null }
... and depend on package being null when the optionSupport should be disabled ...
and then evaluate as .override { package = null; }
This works like the first pattern, but can't be overriden by
`nixpkgs.config`, which I love and use extensively.
or
{ package }
... and depend on package being null when the optionSupport should be disabled ...
and then evaluate as .override { package = null; }
Many people prefer this because it the least amount of typing in the
package expression, but I hate it. `buildInputs` ignoring nulls is a
hack that should be discouraged, IMO.
I do one of the first two whichever it is I feel like ATM. I can either
add an assert, or remove the null. Whichever you prefer, I don't really
care in this case.
|
|
Expressing optional features is a bit clunky atm, to put it midlly, and have poor discoverability, but that's a topic I'm not too keen on rehashing, so if you prefer using this pattern that's fine by me. I just think the defaults are misleading here, by suggesting a combination of param values that causes eval error. |
…the box Before one had to turn it on manually and update the preview script in dotfiles manually when ranger updates. Now it requires zero configuration. Just run `ranger` and it works, and should continue to work automagically when ranger updates. Everything still can be (de)configured via `rc.conf` in dotfiles.
|
I think current version is ok to merge.
|
|
Thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge #26452 first or this won't build.
Motivation for this change
See commit message.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)