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

cloudmonkey: init at 5.3.3 with related dependency argcomplete: init at 1.8.2 #26124

Merged
merged 2 commits into from
May 30, 2017

Conversation

womfoo
Copy link
Member

@womfoo womfoo commented May 26, 2017

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • 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.

@mention-bot
Copy link

@womfoo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh to be a potential reviewer.

@@ -1159,6 +1159,28 @@ in {
};
};

argcomplete = buildPythonPackage rec {
Copy link
Member

Choose a reason for hiding this comment

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

Python modules have to be in the directory pkgs/development/python-modules/ as suggested here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! It's been a while since I last contributed a python library. Just pushed a new commit for this.

@@ -0,0 +1,25 @@
{ python2Packages, lib }:
Copy link
Member

Choose a reason for hiding this comment

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

FYI, it seems no tests are executed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm mostly a python user rather than a developer and couldn't find where to enable the tests for cloudmonkey (nothing in the repo or its setup.py). Any pointers?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'm also not able to find any tests;)

Copy link
Member Author

Choose a reason for hiding this comment

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

No prob!

Copy link
Member

@FRidh FRidh May 29, 2017

Choose a reason for hiding this comment

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

if there are no tests, add doCheck = false; and include a comment explaining why they're disabled

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added. Thanks!

@@ -0,0 +1,25 @@
{ python2Packages, lib }:
Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'm also not able to find any tests;)

meta = with lib; {
description = "Bash tab completion for argparse";
homepage = "https://argcomplete.readthedocs.io";
maintainers = maintainers.womfoo;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@womfoo womfoo force-pushed the init/cloudmonkey-5.3.3 branch 2 times, most recently from 2a56d5d to 26240e2 Compare May 29, 2017 20:03

doCheck = false; # bash-completion test fails with "compgen: command not found".

propagatedBuildInputs = [ coverage dicttoxml flake8 pexpect prettytable requests_toolbelt ];
Copy link
Member

Choose a reason for hiding this comment

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

are all of these runtime dependencies? To me, it seems like some (like flake8) are only needed for tests

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, now moved coverage and flake8 to a commented buildInputs while tests are disabled. I've also manually tested the package and it works fine.

@FRidh FRidh merged commit 89a32f4 into NixOS:master May 30, 2017
@womfoo womfoo deleted the init/cloudmonkey-5.3.3 branch August 14, 2017 23:14
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