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

Submit/stagit gopher #90502

Closed
wants to merge 2 commits into from
Closed

Conversation

hcssmith
Copy link

stagit-gopher init at version 0.9.3

Motivation for this change

So I can use stagit-gopher on Nixos

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Few minor nits, otherwise LGTM

Indentation is off, should be two spaces i think.

Commits don't follow the contributing guidelines (should be squashed, with the message as described in there). Should note that the maintainer list addition should be in a seperate commit as well.

{ stdenv, libgit2, fetchgit }:

stdenv.mkDerivation rec {
name = "stagit-gopher";
Copy link
Member

Choose a reason for hiding this comment

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

You should be using pname here.

makeFlags = [ "PREFIX=$(out)" ];

buildInputs = [ libgit2 ];

Copy link
Member

Choose a reason for hiding this comment

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

Second newline isn't needed

@hcssmith
Copy link
Author

OK @IvarWithoutBones , So I should fix the formatting in the actual code files, squash the commits and do a separate pull request for the maintainers file?

@IvarWithoutBones
Copy link
Member

IvarWithoutBones commented Jun 15, 2020

Yep, except that the maintainer file changes can be in this PR, it should just be in a different commit than the actual package addition.

@hcssmith hcssmith force-pushed the submit/stagit-gopher branch 2 times, most recently from 798c7ab to ee0a570 Compare June 15, 2020 17:22
@hcssmith
Copy link
Author

@IvarWithoutBones, Right think I've got it this time. Maintainer and package in in separate commits and I think I've got the formatting sorted this time. Sorry about the slight ineptitude with getting the commits correctly squashed together. Must have taken about three or four resets

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Sorry about the slight ineptitude with getting the commits correctly squashed together. Must have taken about three or four resets

No worries, git can be a bit confusing sometimes

LGMT :)

Output from nixpkgs-review:

1 package built:
stagit-gopher

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Apologies, looks like i missed a few more nits

url = "git://git.codemadness.org/stagit-gopher";
rev = version;
sha256 = "0dcyi3k900jb8p5qhgfk91gvplblysgfz1vyn6pqrlk931hpac80";
};
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to be wrong here

description = "Static git site generator for gopher";
license = licenses.mit;
platforms = platforms.all;
maintainers = with maintaininers; [ hcssmith ];
Copy link
Member

Choose a reason for hiding this comment

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

Typo here, causing ofborg to fail.

Suggested change
maintainers = with maintaininers; [ hcssmith ];
maintainers = with maintainers; [ hcssmith ];

Copy link
Author

Choose a reason for hiding this comment

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

OK, I've corrected the typo, not sure how i missed that given that I wrote it two words before, and I have moved the } back two spaces which I think is what you meant.

@stale
Copy link

stale bot commented Apr 18, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 18, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 13, 2022
@Artturin Artturin closed this May 6, 2022
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.

None yet

5 participants