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

git-publish: init at 1.8.1 #190309

Merged
merged 1 commit into from
Oct 14, 2022
Merged

Conversation

lheckemann
Copy link
Member

@lheckemann lheckemann commented Sep 8, 2022

Description of changes

Init 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@lheckemann
Copy link
Member Author

Undrafted, I think I'll be using this enough that I can maintain it :)

Comment on lines 1 to 2
{ lib, stdenv, python, perl, fetchFromGitHub }:
stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ lib, stdenv, python, perl, fetchFromGitHub }:
stdenv.mkDerivation rec {
{ lib, stdenv, python, perl, fetchFromGitHub }:
stdenv.mkDerivation rec {

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a written convention for this now? If not I won't apply this.

Copy link
Member

Choose a reason for hiding this comment

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

@teto How about you are not being passive aggressive?

Copy link
Member

Choose a reason for hiding this comment

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

@lheckemann we do not have written rules for literally everything and I don't want to write rules for everything. It would make it a lot easier if you could just incorporate style changes together with the other 3 changes. If it would be the only comment then I would have completely ignored it. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is: I prefer the style that I wrote. As long as we don't have a rule saying what the preferred variant is to resolve matters of pure opinion like this, I don't think it makes sense to have these discussions.

I'm grateful for your reviews that bring my attention to useful things like installManPage -- I think we can all agree that using abstractions like this where they exist makes sense.

But things that are matters of pure opinion, like this blank line here, are a waste of time. I'd appreciate if you could limit yourself to the actually useful things -- or policies that are actually written somewhere, so i don't have to guess what "SuperSandro2000-conforming style" is -- in future reviews of my code.

@SuperSandro2000
Copy link
Member

@lheckemann can you activate that maintainer can push to your branch? That makes things a lot easier and faster to merge.

@SuperSandro2000 SuperSandro2000 merged commit 51c0569 into NixOS:master Oct 14, 2022
@lheckemann lheckemann deleted the git-publish branch October 15, 2022 00:59
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.

3 participants