-
-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
twm: 0.9.0 -> 0.9.1, add updateScript #312304
Conversation
Result of 1 package built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution, @vinnymeller, please find below a few suggestions for improvement.
pkgs/tools/misc/twm/default.nix
Outdated
}: | ||
|
||
rustPlatform.buildRustPackage rec { | ||
pname = "twm"; | ||
version = "0.9.0"; | ||
version = "0.9.1"; | ||
|
||
src = fetchFromGitHub { | ||
owner = "vinnymeller"; | ||
repo = pname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a preference to avoid such use of pname
(see #277994)
repo = pname; | |
repo = "twm"; |
pkgs/tools/misc/twm/default.nix
Outdated
|
||
src = fetchFromGitHub { | ||
owner = "vinnymeller"; | ||
repo = pname; | ||
rev = "v${version}"; | ||
sha256 = "sha256-gvo5+lZNe5QOHNI4nrPbCR65D+VFf/anmLVdu5RXJiY="; | ||
sha256 = "sha256-1hRGcvUGHO5ENgI3tdT46736ODN8QZ6vg+Y1y2XeuAA="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using hash
is preferred over sha256
:
sha256 = "sha256-1hRGcvUGHO5ENgI3tdT46736ODN8QZ6vg+Y1y2XeuAA="; | |
hash = "sha256-1hRGcvUGHO5ENgI3tdT46736ODN8QZ6vg+Y1y2XeuAA="; |
pkgs/tools/misc/twm/default.nix
Outdated
|
||
nativeBuildInputs = [ pkg-config ]; | ||
buildInputs = [ openssl ] ++ lib.optionals stdenv.isDarwin [ Security ]; | ||
|
||
passthru.updateScript = nix-update-script { }; | ||
|
||
meta = with lib; { | ||
description = "A customizable workspace manager for tmux"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indefinite articles at the beginning of the package description provide no value.
description = "A customizable workspace manager for tmux"; | |
description = "Customizable workspace manager for tmux"; |
f925cae
to
5f6320f
Compare
pkgs/tools/misc/twm/default.nix
Outdated
|
||
nativeBuildInputs = [ pkg-config ]; | ||
buildInputs = [ openssl ] ++ lib.optionals stdenv.isDarwin [ Security ]; | ||
|
||
passthru.updateScript = nix-update-script { }; | ||
|
||
meta = with lib; { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta = with lib; { | |
meta = { |
5f6320f
to
e262aa3
Compare
e262aa3
to
0def979
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for considering and accepting the review suggestions.
LGTM 馃檪馃憤
pkgs/tools/misc/twm/default.nix
Outdated
passthru.updateScript = nix-update-script { }; | ||
|
||
meta = { | ||
description = "Customizable workspace manager for tmux"; | ||
homepage = "https://github.com/vinnymeller/twm"; | ||
changelog = "https://github.com/vinnymeller/twm/releases/tag/v${version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hint:
changelog = "https://github.com/vinnymeller/twm/releases/tag/v${version}"; | |
changelog = "https://github.com/vinnymeller/twm/releases/tag/${src.rev}"; |
Because sometimes the upstream changes versioning scheme.
Also it works when we use a release without its git tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this pattern before and always wondered what the benefits might be. Thanks for adding context on this, @AndersonTorres 馃憤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, LGTM.
0def979
to
7386121
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1657 |
Description of changes
Things 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.