search

madjar / nox

Tools to make nix nicer to use
MIT License
305 stars 35 forks source link

nox-review: fix `wip` click decorator #80

Open lsix opened 6 years ago

lsix commented 6 years ago

Since f50bebb4783c60ab50875eb8383d921e2df014b7, the function the @cli.command decorator sees is the one generated with @setup_nixpkgs_config, i.e. _ (c.f. nox/review.py:66). Therefore it cannot properly guess figure the wip function name.

This explains the behaviour observed in https://github.com/NixOS/nixpkgs/issues/29684:

$ nox-review --help
Usage: nox-review [OPTIONS] COMMAND [ARGS]...

  Review a change by building the touched commits

Options:
  -k, --keep-going  Keep going in case of failed builds
  --dry-run         Don't actually build packages, just print the
commands
                    that would have been run
  --help            Show this message and exit.

Commands:
  _   difference between working tree and a commit
  pr  changes in a pull request

This commit basically does what is done in nixpkgs to fix the behaviour change: https://github.com/NixOS/nixpkgs/commit/5f0a7cded718d0092229ba078cb7f0f7733bf636

bobvanderlinden commented 6 years ago

I ran into this problem on nixpkgs. The documented command nix-shell -p nox --run "nox-review wip" currently shows an error:

Usage: nox-review [OPTIONS] COMMAND [ARGS]...

Error: No such command "wip".

With the current state of nox only nox-review _ will work. This PR is very much appreciated and it would be great to get it merged and into nixpkgs.

lsix commented 6 years ago

@bobvanderlinden there is already a fix in nixpkgs (https://github.com/NixOS/nixpkgs/commit/5f0a7cded718d0092229ba078cb7f0f7733bf636). I just ported it to stable so as soon as the channel has been updated by hydra it will be fixed in release-17.09 as far as the users are concerned.

bobvanderlinden commented 6 years ago

Ah, great. Thank you!

madjar commented 6 years ago

Thanks for that!

I can't merge it right now, because that would break the CI (the default.nix depends on nixpkgs, so it tries to apply that patch). I'll merge this just before I make the next release.