nlohmann_json_schema_validator: init at 2.3.0 #275224

br337 · 2023-12-18T17:03:48Z

Description of changes

This package is a JSON schema validator for Modern C++. More information on it here.

Things done

Add a 👍 reaction to pull requests you find important.

drupol

Hi !

Thanks for your first contribution!

Things left to do:

Move your file in pkgs/by-name/... and remove the entry from all-packages.nix
The PR title should be: <name>: init at <version>
The commit message should be identical to the PR title
It would be nice to have you as maintainer, therefore a commit to add yourself in the maintainers file (maintainers: add br337) needs to be made before the current commit.
The pname looks wonky, why is it containing the name of one of its dependency ?

Let me know if you need some help!

br337 · 2023-12-18T19:39:23Z

Hi !

Thanks for your first contribution!

Things left to do:

1. Move your file in `pkgs/by-name/...` and remove the entry from `all-packages.nix`

2. The PR title should be: `<name>: init at <version>`

3. The commit message should be identical to the PR title

4. It would be nice to have you as maintainer, therefore a commit to add yourself in the maintainers file (`maintainers: add br337`) needs to be made before the current commit.

5. The `pname` looks wonky, why is it containing the name of one of its dependency ?

Let me know if you need some help!

Hi, I'm sorry if the following questions will seem kind of silly but I struggle with some things regarding this PR:

Why should this package go in pkgs/by-name/ and not in pkgs/development/libraries/ since it is a C++ dev library closely related to nlohmann_json which already resides in pkgs/development/libraries/?
<name> here being the name of the package and <version> the version of said library?
Noted! 😃
Sure! That would be great. Would that require an additional PR or should that be done in this PR but just as an additional commit before the package has been committed? I assume I should add myself in the /maintainers/maintainer-list.nix file.
I know it's wonky, but I'm not the original developer of the library and that is sadly the name the library uses pretty much everywhere, including in CMake when importing it.

drupol · 2023-12-18T20:14:33Z

Hi, I'm sorry if the following questions will seem kind of silly but I struggle with some things regarding this PR:
1. Why should this package go in `pkgs/by-name/` and not in `pkgs/development/libraries/` since it is a C++ dev library closely related to `nlohmann_json` which already resides in `pkgs/development/libraries/`?

This is the new package name convention (RFC140). Maybe you should have a look and see if there are already libraries in there. If yes, then I would kindly ask you to follow that pattern.

2. `<name>` here being the name of the package and `<version>` the version of said library?

Correct.

4. Sure! That would be great. Would that require an additional PR or should that be done in this PR but just as an additional commit before the package has been committed? I assume I should add myself in the `/maintainers/maintainer-list.nix` file.

Yes, you can make a commit within this PR with you added in the maintainers list.

5. I know it's wonky, but I'm not the original developer of the library and that is sadly the name the library uses pretty much everywhere, including in CMake when importing it.

Fair enough, since I don't know anything in there, I'll trust you in there.

maintainers/maintainer-list.nix

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix

Fixed attribute order in maintainers list

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix

br337 · 2023-12-18T21:14:53Z

Should I also squash the last commits?

drupol · 2023-12-18T21:15:13Z

Should I also squash the last commits?

Correct.

br337 · 2023-12-18T21:26:32Z

Should I also squash the last commits?

Correct.

Done

drupol

Congrats for your first PR, welcome to the family !
It LGTM, but let's wait for someone else's review.

Keep PRs coming!

pbsds

Result of nixpkgs-review pr 275224 run on x86_64-linux 1

1 package built:

nlohmann_json_schema_validator

Runs fine, LGTM. Just a nit

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix

2xsaiko

Just a small thing :^)

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix

2xsaiko · 2023-12-21T11:23:04Z

1 package built:
nlohmann_json_schema_validator

Result of nixpkgs-review pr 275224 run on aarch64-darwin 1

1 package built:

nlohmann_json_schema_validator

br337 · 2023-12-22T10:22:33Z

Changes are now squashed

Janik-Haag · 2023-12-22T15:33:37Z

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix

+  meta = {
+    description = "JSON schema validator for JSON for Modern C++";
+    homepage = "https://github.com/pboettch/json-schema-validator";
+    license = lib.licenses.mit;
+    maintainers = with lib.maintainers; [ br337 ];
+    platforms = lib.platforms.all;
+  };


please add meta.mainProgram, since the package adds multiple binarys and things like lib.getExe or nix run won't work without it.

Unless there is a clear binary the package provides which is the one users expect to be run, i oppose setting mainProgram. In such cases one is expected to use lib.getExe' or nix shell PKG -c binary ....

Being able to disable the bin/${pname} fallback behavior of lib.getExe with something like mainProgram = null would be great, but implementing that that is outside the scope of this PR

Unless there is a clear binary the package provides which is the one users expect to be run, i oppose setting mainProgram. In such cases one is expected to use lib.getExe' or nix shell PKG -c binary ....

I would have set it to json-schema-validate since that's the thing in the package name. But feel free to merge if you think otherwise, the package looks good to me except for this.

So should I implement this change now or should I resolve the issue without further changes?

drupol · 2023-12-26T21:35:17Z

Let's not wait too long before getting feedback, especially on a first contributor PR.
If anything need to be changed now, it can be done either by the author or someone else.

Thanks for your contribution, keep PRs coming!

NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 18, 2023

drupol requested changes Dec 18, 2023

View reviewed changes

br337 closed this Dec 18, 2023

br337 force-pushed the nixos-unstable branch from 1084396 to a9bf124 Compare December 18, 2023 20:35

br337 reopened this Dec 18, 2023

br337 changed the title ~~Added package for nlohmann-json-schema-validator~~ nlohmann_json_schema_validator: init at 2.3.0 Dec 18, 2023

br337 force-pushed the nixos-unstable branch 2 times, most recently from 19aea87 to 84af350 Compare December 18, 2023 20:45

drupol requested changes Dec 18, 2023

View reviewed changes

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix Outdated Show resolved Hide resolved

drupol changed the base branch from nixos-unstable to master December 18, 2023 20:50

maintainers: add br337

d5f7b82

Fixed attribute order in maintainers list

br337 force-pushed the nixos-unstable branch 2 times, most recently from 86bbbd3 to 0661c76 Compare December 18, 2023 21:05

drupol requested changes Dec 18, 2023

View reviewed changes

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix Outdated Show resolved Hide resolved

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix Outdated Show resolved Hide resolved

drupol reviewed Dec 18, 2023

View reviewed changes

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix Outdated Show resolved Hide resolved

br337 force-pushed the nixos-unstable branch from 0f7813b to c35b231 Compare December 18, 2023 21:16

br337 marked this pull request as ready for review December 18, 2023 21:18

drupol approved these changes Dec 18, 2023

View reviewed changes

delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 18, 2023

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 19, 2023

pbsds approved these changes Dec 20, 2023

View reviewed changes

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix Show resolved Hide resolved

2xsaiko suggested changes Dec 21, 2023

View reviewed changes

pkgs/by-name/nl/nlohmann_json_schema_validator/package.nix Outdated Show resolved Hide resolved

br337 force-pushed the nixos-unstable branch from 2b61b89 to 162a921 Compare December 22, 2023 10:21

br337 requested review from piegamesde and Janik-Haag as code owners December 22, 2023 10:21

nlohmann_json_schema_validator: init at 2.3.0

f6b24b3

delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 22, 2023

br337 force-pushed the nixos-unstable branch from 162a921 to f6b24b3 Compare December 22, 2023 10:22

drupol approved these changes Dec 22, 2023

View reviewed changes

delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 22, 2023

2xsaiko approved these changes Dec 22, 2023

View reviewed changes

delroth added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Dec 22, 2023

piegamesde removed request for piegamesde and Janik-Haag December 22, 2023 14:39

Janik-Haag reviewed Dec 22, 2023

View reviewed changes

delroth added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people labels Dec 23, 2023

drupol merged commit cacf372 into NixOS:master Dec 26, 2023
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nlohmann_json_schema_validator: init at 2.3.0 #275224

nlohmann_json_schema_validator: init at 2.3.0 #275224

br337 commented Dec 18, 2023 •

edited

Loading

drupol left a comment

br337 commented Dec 18, 2023

drupol commented Dec 18, 2023

br337 commented Dec 18, 2023

drupol commented Dec 18, 2023

br337 commented Dec 18, 2023

drupol left a comment

pbsds left a comment

2xsaiko left a comment

2xsaiko commented Dec 21, 2023

br337 commented Dec 22, 2023

Janik-Haag Dec 22, 2023 •

edited

Loading

pbsds Dec 23, 2023 •

edited

Loading

pbsds Dec 23, 2023

Janik-Haag Dec 23, 2023

br337 Dec 25, 2023

drupol commented Dec 26, 2023

nlohmann_json_schema_validator: init at 2.3.0 #275224

nlohmann_json_schema_validator: init at 2.3.0 #275224

Conversation

br337 commented Dec 18, 2023 • edited Loading

Description of changes

Things done

drupol left a comment

Choose a reason for hiding this comment

br337 commented Dec 18, 2023

drupol commented Dec 18, 2023

br337 commented Dec 18, 2023

drupol commented Dec 18, 2023

br337 commented Dec 18, 2023

drupol left a comment

Choose a reason for hiding this comment

pbsds left a comment

Choose a reason for hiding this comment

2xsaiko left a comment

Choose a reason for hiding this comment

2xsaiko commented Dec 21, 2023

br337 commented Dec 22, 2023

Janik-Haag Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

pbsds Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

pbsds Dec 23, 2023

Choose a reason for hiding this comment

Janik-Haag Dec 23, 2023

Choose a reason for hiding this comment

br337 Dec 25, 2023

Choose a reason for hiding this comment

drupol commented Dec 26, 2023

br337 commented Dec 18, 2023 •

edited

Loading

Janik-Haag Dec 22, 2023 •

edited

Loading

pbsds Dec 23, 2023 •

edited

Loading