-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
jugglinglab: init at 1.2 #87875
jugglinglab: init at 1.2 #87875
Conversation
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.
Hello, there :)
Thanks for taking the time to contribute a package to nixpkgs. I reviewed your PR and found a few small issues, which I added as comments. Here's a summary following the review guide for new packages.
Reviewed points
- package path fits guidelines
- package name fits guidelines
- package version fits guidelines (but not up-to-date)
- package build on x86_64, NixOS
- executables tested on x86_64, NixOS
-
meta.description
is set and fits guidelines -
meta.license
fits upstream license -
meta.platforms
is set MISSING -
meta.maintainers
is set - build time only dependencies are declared in
nativeBuildInputs
- source is fetched using the appropriate function (no, fetchgit used for GitHub repo)
- phases are respected
- patches that are remotely available are fetched with
fetchpatch
(no patches)
Possible improvements
see code comments
stdenv.mkDerivation rec { | ||
major= "1"; | ||
minor = "2"; | ||
version = "${major}.${minor}"; |
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.
Looks like the most recent version is 1.2.1 and the program gives me a pop-up saying that there is an update. So I would suggest an update ;)
Minor nit-pick: since you are not really using major/minor version info for anything other than the version, I would collapse this down to version = "1.2.1"
, as it makes the code easier to follow.
pkgs/top-level/all-packages.nix
Outdated
@@ -7204,6 +7204,8 @@ in | |||
|
|||
umlet = callPackage ../tools/misc/umlet { }; | |||
|
|||
jugglinglab = callPackage ../tools/misc/jugglinglab { }; |
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.
For consistency, please sort this alphabetically.
minor = "2"; | ||
version = "${major}.${minor}"; | ||
name = "jugglinglab"; | ||
src = fetchgit { |
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.
We prefer fetchFromGitHub
over fetchgit
for GitHub repositories.
meta = with stdenv.lib; { | ||
description = "A program to visualize different juggling pattens"; | ||
license = licenses.gpl2; | ||
maintainers = with maintainers; [ wnklmnn ]; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Reviewed points
- package path fits guidelines
- package name fits guidelines
- package version fits guidelines
- package build on NixOS x86_64
- executables tested on NixOS x86_64
-
meta.description
is set and fits guidelines -
meta.license
fits upstream license -
meta.platforms
is set -
meta.maintainers
is set - build time only dependencies are declared in
nativeBuildInputs
- source is fetched using the appropriate function
- phases are respected
- patches that are remotely available are fetched with
fetchpatch
(none)
Possible improvements
none
Comments
👍 looks good to me!
LGTM @wnklmnn Thanks for sending this PR, and we look forward to more contributions from you in the future! @aepsil0n Thanks for doing a quick review of this. |
Motivation for this change
Adds jugglinglab. A program to visualize different juggling patterns.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)