sphalerite	I've submitted a PR for a new package where I used a way of specifying the version which isn't used widely in nixpkgs AFAIK and which I suspected might raise some eyebrows but I think it has some concrete advantages and would like some more input on the idea in general.
sphalerite	The idea is that rather than having a let binding or passing a rec set to stdenv.mkDerivation, we make the version string and source hash arguments to the function to make overriding the version a lot simpler. See https://github.com/NixOS/nixpkgs/pull/34275#discussion_r164206558
sphalerite	Any feedback would be very welcome!
tilpner	sphalerite - I'm generally against putting non-dependency arguments there, it can cause hard-to-debug issues with callPackage. And it should probably be sourceSha256
sphalerite	tilpner: hard-to-debug issues with callPackage? Can you elaborate on that? And yes I suppose you're right with the name, that's of course just a minor detail independent of the concept.
sphalerite	tilpner: do you have an idea for how the redundancy might be avoided more nicely?
tilpner	sphalerite - It's fairly innocuous in your case, but it's dangerous if it becomes convention. ncurses takes an argument "unicdoe", and if you happen to overlay that onto your local nixpkgs, it will break with unhelpful errors. It will supply whatever you overlayed as unicode to ncurses, which expects a bool there
tilpner	sphalerite_ - The same happens when you overlay "vanilla", though I forgot which package caused that
sphalerite	tilpner: right, fair enough. And do you have a potential better idea for how to do this?
tilpner	sphalerite - A while ago I proposed fixing this issue gradually, converting every package into two stages of loading, one for dependencies, the next set for configuration. It's a lot of effort though, and nobody wanted to discuss it further
tilpner	sphalerite - A workaround would be to clearly mark them as configuration arguments. unicode -> supportUnicode, vanilla -> useVanilla
tilpner	sphalerite - In your case, srcVersion and srcSha256 maybe? Kind of ugly, yes...
sphalerite	tilpner: that's a shame. Seems like something that might fit with nbp's SOS as well
sphalerite	I think
sphalerite	tilpner: so you think the idea is ok in general, just the names need to be specific enough to avoid a collision?
tilpner	sphalerite - I'm not sold on it. I think I'd rather have otter-browser.overrideAttrs (o: { src = o.src.override { rev = ...; sha256 = ...; }; }) (requires makeOverridable fetchFromGitHub)
sphalerite	tilpner: but then the version bit is still redundant and you're likely to get a derivation with the wrong name resulting from it — because you forget to include it in the override like you did right there
tilpner	sphalerite - Good point :/
sphalerite	tilpner: I find that a lot of the time there are `let`s in nixpkgs which introduce abstractions that can't be used from outside. Arguably allowing them to be used from the outside encourages more tight coupling, but at the same time I don't think there's much in nixpkgs that isn't tightly coupled. Between tight coupling and better potential for reuse (=> less code) and tight coupling and lots of copying and pasting…
sphalerite	`let`s and `rec`s even
clever	sphalerite: what would you say about code like this? https://github.com/NixOS/nixpkgs/blob/8080285966835c202ed426a6f573fe1f53aa0d08/pkgs/games/zandronum/default.nix#L8
tilpner	Why is fmod not a global package there?
clever	tilpner: exactly
tilpner	I guess it's fine if they require custom (old) or patched versions of their libs
tilpner	Don't really want those in global scope
clever	tilpner: there is no other version of fmod available in nixpkgs
tilpner	clever - That's a mistake then :/
clever	yeah
tilpner	sphalerite_ - Most of the time, I think it's okay. Using callPackage to supply args instead of pkgs.foo already decouples
tilpner	sphalerite_ - Of course nixpkgs is still pretty messy, but big overhauls are probably unrealistic