Mednafen refactors #304942
Mednafen refactors #304942
Conversation
- finalAttrs - strictDeps
- finalAttrs - strictDeps
- finalAttrs - strictDeps
Co-authored-by: R. RyanTM <[email protected]>
There was a problem hiding this comment.
Good start, though needs some improvements. Another thing to note, please stick with the git commit naming conventions we use. You can use git rebase -i to squash and rename commits so the history looks cleaner and you can follow the convention.
| wrapGAppsHook, | ||
| }: | ||
|
|
||
| stdenv.mkDerivation (finalAttrs: { |
There was a problem hiding this comment.
Why (finalAttrs: { instead of just rec {?
There was a problem hiding this comment.
Because the majority of other packages use rec and achieve the same result as what wrapping the attributes in a function does. Also, what does this change fix or improve that required the removal of rec and wrapping it in a function?
There was a problem hiding this comment.
Because the majority of other packages use rec
Popularity does not make a good argument here, or else things like treewide banishing unquoted URLs from the codebase would never happen.
and achieve the same result as what wrapping the attributes in a function does
Although being spread over the whole Nixpkgs codebase, rec is an anti-pattern:
https://nix.dev/guides/best-practices#recursive-attribute-set-rec
Then, finalAttrs design pattern (a.k.a. overlay style overridable recursive attributes) was merged more than 3 years ago, to overcome some of the problems of rec.
Also, what does this change fix or improve that required the removal of rec and wrapping it in a function?
There was a problem hiding this comment.
Popularity does not make a good argument here, or else things like treewide banishing unquoted URLs from the codebase would never happen.
I am aware, however I believe the change should be made with a purpose.
Although being spread over the whole Nixpkgs codebase, rec is an anti-pattern:
https://nix.dev/guides/best-practices#recursive-attribute-set-rec
Then,
finalAttrsdesign pattern (a.k.a. overlay style overridable recursive attributes) was merged more than 3 years ago, to overcome some of the problems ofrec.
Is there an RFC or issue about treewide removal of rec? The way it is upstream works fine currently. That nix.dev page recommends using let...in as an option.
There was a problem hiding this comment.
I am aware, however I believe the change should be made with a purpose.
"Getting rid of rec (and other bad design patterns)" is a purpose, I suppose.
Is there an RFC or issue about treewide removal of rec?
Reasoning backwards it is.
First, technically there is no need of a "treewide-rec-banish RFC" to banish rec from Nixpkgs.
Second, refactoring the expressions I maintain is in my discretion.
Third, objectively the code is not worse than before; it is still perfectly readable and understandable.
Fourth, and related to second, subjectively the code is better since it avoids pitfalls from bad design patterns, and it is better to me to maintain and improve it.
The way it is upstream works fine currently.
Not a sufficient reason to hinder code refactoring.
That nix.dev page recommends using let...in as an option.
Yes, and I do use it sometimes.
But this nix.dev tutorial recommends let-in because, among other things, its purpose is to use more general approaches, especially in external codebases that will not necessarily have direct access to Nixpkgs.
Also, things like #119942 did not exist in that time.
There was a problem hiding this comment.
Alright, I understand. 👍
| fetchurl, | ||
| }: | ||
|
|
||
| stdenv.mkDerivation (finalAttrs: { |
There was a problem hiding this comment.
Same here. Why (finalAttrs: { instead of just rec {?
| install -m 644 -Dt $out/share/mednafen-server standard.conf | ||
| ''; | ||
|
|
||
| meta = { |
There was a problem hiding this comment.
If you make meta = with lib; { then you can remove lib. from anything below this line.
There was a problem hiding this comment.
with does not work like the distributive axiom of a field (as suggested by your remark), and its rules are unintuitive.
There was a problem hiding this comment.
That last one is about overuse of with. In this case, I wouldn't consider using with lib; for assigning values in meta to be an overuse.
There was a problem hiding this comment.
Until a nasty with scope invasion appears:
| pname = "mednafen"; | ||
| version = "1.29.0"; | ||
|
|
||
| src = fetchurl { | ||
| url = "https://mednafen.github.io/releases/files/${pname}-${version}.tar.xz"; | ||
| url = "https://mednafen.github.io/releases/files/mednafen-${finalAttrs.version}.tar.xz"; |
There was a problem hiding this comment.
Why remove ${pname} and replace it with the literal package name?
Description of changes
Closes #304028
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.