Skip to content
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

ocaml-protoc-plugin: init at 4.3.1 #248155

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

GirardR1006
Copy link

@GirardR1006 GirardR1006 commented Aug 9, 2023

Description of changes

Adding ocaml-protoc-plugin, an Ocaml library to convert Google protobuf format into Ocaml types.

Project homepage: https://github.com/issuu/ocaml-protoc-plugin

I am not the original author, but I find this tool convenient enough to be added into nixpkgs. I will gladly be added as a maintainer for this package.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you PR. I left a few comments mostly about style/metadata, but the package itself seems to build fine :)

Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please add yourself to the list in one commit, and then modify the package in one other. This is to ensure we can revert the commit affecting the package without breaking evaluation even if you add yourself as maintainer to other packages in the meantime.

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi !

Thanks for your contribution.

I've left some comments, left me know if this is clear enough.

@drupol
Copy link
Contributor

drupol commented Aug 15, 2023

Moving this PR in draft until the feedback has been addressed, feel free to remove it from draft when it's done, so we can see it again appearing in our list of PRs to review. Thanks!

@drupol drupol marked this pull request as draft August 15, 2023 08:54
@GirardR1006
Copy link
Author

Moving this PR in draft until the feedback has been addressed, feel free to remove it from draft when it's done, so we can see it again appearing in our list of PRs to review. Thanks!

Thanks for that, I'll address your feedbacks during the week :)

@GirardR1006 GirardR1006 marked this pull request as ready for review August 16, 2023 16:20
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please ensure that your PR only contains 2 commits?

  • 1 commit for adding you in the maintainers list: maintainers: add GirardR1006
  • 1 commit for the rest: ocaml-protoc-plugin: init at 4.3.1

@GirardR1006
Copy link
Author

Can you please ensure that your PR only contains 2 commits?

* 1 commit for adding you in the maintainers list: `maintainers: add GirardR1006`

* 1 commit for the rest: `ocaml-protoc-plugin: init at 4.3.1`

That should be it. Sorry for the force pushes, I believe they are making the review progress less trackable. Taking any advices on how to do that better for future contributions :)

@drupol
Copy link
Contributor

drupol commented Aug 16, 2023

No worries for the force-pushed, it's mandatory here, and it's not considered as a bad practice as long as you stay in your branch, hopefully.

At the moment, the CONTRIBUTING.md file is being rewritten, but you can already find some leads in there.

For the rest, the commiters/maintainers are here to review and provide feedback.

Feel free to ask anything, we'll be there to help.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last minor thing and it's good to go for me.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, thanks for implementing all the change requests!

@vbgl vbgl merged commit 8d96cbf into NixOS:master Aug 17, 2023
14 of 16 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants