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

ansifilter: add @ 1.15 #14023

Merged
merged 1 commit into from
Mar 22, 2016
Merged

ansifilter: add @ 1.15 #14023

merged 1 commit into from
Mar 22, 2016

Conversation

Adjective-Object
Copy link
Contributor

Things done:
  • Tested using sandboxing (nix-build --option build-use-chroot true or nix.useChroot on NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.
More

added package ansifilter @ 1.15

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @zimbatm, @edolstra and @vcunat to be potential reviewers

let version = "1.15";
pkgsha = "65dc20cc1a03d4feba990f830186404c90462d599e5f4b37610d4d822d67aec4";
in with pkgs; stdenv.mkDerivation {
name = "ansifilter";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make version a part of the name, as in name = ansifilter-${version}.

@joachifm
Copy link
Contributor

Please change the commit subject to ansifilter: init at 1.5.

@joachifm joachifm added the 8.has: package (new) This PR adds a new package label Mar 18, 2016
@@ -12,6 +12,7 @@
abbradar = "Nikolay Amiantov <ab@fmap.me>";
aboseley = "Adam Boseley <adam.boseley@gmail.com>";
adev = "Adrien Devresse <adev@adev.name>";
adjective = "Maxwell Huang-Hobbs <mhuan13@gmail.com>";
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice if you'd re-use your GitHub handle here

@Adjective-Object
Copy link
Contributor Author

Sorry about the description! That's embarrassing.

I implemented the changes you suggested, apart from the makeFlags change. I encountered a weird error where the package build wouldn't actually produce anything if the make flags were specified with makeFlags instead of through configurePhase.

This method of passing makeFlags is modeled after the highlight package, from the same people who made ansifilter.

@joachifm
Copy link
Contributor

Did you try the $(out) syntax for makeFlags? They are being passed verbatim to make, so expansion occurs within make, not the shell (thus, $out won't work).

@@ -12,6 +12,7 @@
abbradar = "Nikolay Amiantov <ab@fmap.me>";
aboseley = "Adam Boseley <adam.boseley@gmail.com>";
adev = "Adrien Devresse <adev@adev.name>";
adjective-object = "Maxwell Huang-Hobbs <mhuan13@gmail.com>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it the exact same as your github handle; this is to make it easier to ping you on github if necessary

@Adjective-Object
Copy link
Contributor Author

Updated maintainer name and used $(out) syntax, sorry for the trouble

sha256 = pkgsha;
};
meta = {
description = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: please remove the newline at the start here; if you want to provide a longer form description, use longDescription.

@joachifm
Copy link
Contributor

No trouble. Apart from the final nitpick, I think this is ready to merge.

@joachifm
Copy link
Contributor

Merged, thanks

joachifm added a commit that referenced this pull request Mar 22, 2016
@joachifm joachifm merged commit 4f29a8c into NixOS:master Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants