jugglinglab: init at 1.2 #87875
jugglinglab: init at 1.2 #87875
Conversation
There was a problem hiding this comment.
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.descriptionis set and fits guidelines -
meta.licensefits upstream license -
meta.platformsis set MISSING -
meta.maintainersis 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.
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.
For consistency, please sort this alphabetically.
| minor = "2"; | ||
| version = "${major}.${minor}"; | ||
| name = "jugglinglab"; | ||
| src = fetchgit { |
There was a problem hiding this comment.
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.
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.descriptionis set and fits guidelines -
meta.licensefits upstream license -
meta.platformsis set -
meta.maintainersis 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!
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
and removed
10.rebuild-darwin: 0
labels
|
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
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)