-
-
Notifications
You must be signed in to change notification settings - Fork 16.6k
ckb: init at 0.202.0 #430842
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
base: master
Are you sure you want to change the base?
ckb: init at 0.202.0 #430842
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.
Moin and welcome to Nixpkgs. Here's a couple recommendations and common nitpicks. Make sure to amend/squash your edits to the init commit, including formatting (my suggestions don't include excessive whitespace etc)
|
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.
GG. Now you need to wait for someone with merge permissions
|
||
cargoHash = "sha256-R7XV//e6n6afbad7HgwlcNwQRdO0xFZ0UGy1SU294Fs="; | ||
|
||
buildFeatures = lib.optionals (stdenv.hostPlatform.isDarwin) [ "portable" ]; |
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.
No need to have parens around a single value:
buildFeatures = lib.optionals (stdenv.hostPlatform.isDarwin) [ "portable" ]; | |
buildFeatures = lib.optionals stdenv.hostPlatform.isDarwin [ "portable" ]; |
Personally, I'd format this with a multiline list, in anticipation of additional build features being specified in the future:
buildFeatures = lib.optionals (stdenv.hostPlatform.isDarwin) [ "portable" ]; | |
buildFeatures = lib.optionals stdenv.hostPlatform.isDarwin [ | |
"portable" | |
]; |
But these are both nits (and/or preferences), and aren't critical.
nativeInstallCheckInputs = [ versionCheckHook ]; | ||
|
||
meta = { | ||
description = "The Nervos CKB is a public permissionless blockchain, and the layer 1 of Nervos network."; |
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.
This is minor, but blocking:
description = "The Nervos CKB is a public permissionless blockchain, and the layer 1 of Nervos network."; | |
description = "Public permissionless blockchain, and the layer 1 of Nervos network"; |
See https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#meta-attributes
- Not start with the definite or an indefinite article.
- Not start with the package name.
- More generally, it should not refer to the package name.
- Not end with a period (or any punctuation for that matter).
In this case "the" is a definite article.
(Note, if needed you can include more detail and/or examples in meta.longDescription
)
Adds new package for https://github.com/nervosnetwork/ckb. a public blockchain.
Things done
passthru.tests
.nixpkgs-review
on this PR. See nixpkgs-review usage../result/bin/
.Add a 👍 reaction to pull requests you find important.