Also your commit messages should be formatted like

pythonPackages.fuzzywuzzy: init at 0.16.0

See: https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md#submitting-changes

@worldofpeace

Also your commit messages should be formatted like ....

Force push or new pr?

@worldofpeace

Also your commit messages should be formatted like ....

Force push or new pr?

Force push for sure, though I would do all of that after you're sure you've addressed the review.
GitHub likes to mark things as outdated I think once that's done.

Yeah I was planning on resolving everything and then squashing into commits with the proper names after everything was resolved. Fixes coming after I finish work today.

@worldofpeace could you take a look at this one more time before I squash into properly named commits?

I looked at the source code and they want either the ledger or hledger executables in PATH.
So i think adding either of those to propagatedBuildInputs should suffice.

Though I'm not sure if users would want both of them? I did notice that there's both ledger-autosync and hledger-autosync executables.

Also I figured a way to get tests "working" after reading https://github.com/egh/ledger-autosync/tree/ddb24c11c26ee36b4b96eac31665eda0d34621e6#testing

Adding

checkInputs = with python3Packages; [ ledger hledger nose mock ];

checkPhase = ''
  nosetests -a generic -a hledger
'';

should work.

I opened an issue yesterday about the license inconsistency in ledger-autosync. The author responded and confirmed that is is in fact GPL3 egh/ledger-autosync#60 (comment)

@worldofpeace

I looked at the source code and they want either the ledger or hledger executables in PATH.
So i think adding either of those to propagatedBuildInputs should suffice.

Though I'm not sure if users would want both of them? I did notice that there's both ledger-autosync and hledger-autosync executables.

Also I figured a way to get tests "working" after reading https://github.com/egh/ledger-autosync/tree/ddb24c11c26ee36b4b96eac31665eda0d34621e6#testing

Adding

checkInputs = with python3Packages; [ ledger hledger nose mock ];

checkPhase = ''
  nosetests -a generic -a hledger
'';

should work.

I did this but also ran it against ledger. The two programs differ in output, but ledger-autosync can talk to them both. I think its best to test against both as the default test suite does.

@worldofpeace thanks for all your feedback on this. I've learned a lot! Can I get a final thumbs-up before I squash and force-push?

@worldofpeace thanks for all your feedback on this. I've learned a lot! Can I get a final thumbs-up before I squash and force-push?

@eamsden You're very welcome 😄

One concern left unaddressed is

I looked at the source code and they want either the ledger or hledger executables in PATH.

We'd like to explicitly declare this dependency and not rely on any global state.
This is important for having a reproducible setup.

So what we could do is add an optional parameter to ledger-autosync where someone could enable which program/s they'd want. Also default to using just ledger.

Maybe like?

diff --git a/pkgs/applications/office/ledger-autosync/default.nix b/pkgs/applications/office/ledger-autosync/default.nix
index 4cddf5c904b..98f90934de8 100644
--- a/pkgs/applications/office/ledger-autosync/default.nix
+++ b/pkgs/applications/office/ledger-autosync/default.nix
@@ -1,4 +1,4 @@
-{ stdenv, python3Packages, fetchFromGitHub, ledger, hledger }:
+{ stdenv, python3Packages, fetchFromGitHub, ledger, hledger, withLedger ? true, withHledger ? false }:
 
 python3Packages.buildPythonApplication rec {
   pname = "ledger-autosync";
@@ -31,7 +31,8 @@ python3Packages.buildPythonApplication rec {
     pycparser  
     secretstorage  
     six  
-  ];  
+  ] ++ stdenv.lib.optionals withLedger [ ledger ]
+    ++ stdenv.lib.optionals withHledger [ hledger ];
   
   # Checks require ledger as a python package,
   # ledger does not support python3 while ledger-autosync requires it.

Though I think that could look simpler.

I'm having trouble finding documentation for the optionals function. With the derivation written as discussed, what would the package user have to do to choose ledger or hledger?

I'm having trouble finding documentation for the optionals function

optionals is documented here. Many others there also.

With the derivation written as discussed, what would the package user have to do to choose ledger or hledger?

They could do this on NixOS for example

environment.systemPackages = [ 
  (pkgs.ledger-autosync.override {
    withLedger = false;
    withHledger = true;
  }) 
];

Would it be ok to take ledger and hledger as arguments, but check if they are non-null before adding them to the dependency list? Then the user could opt against either by overriding e.g.
pkgs.ledger-autosync.override { ledger = null; } or just override the dependency itself.

Then the user could opt against either by overriding e.g.
pkgs.ledger-autosync.override { ledger = null; } or just override the dependency itself.

But we need both for the checks...

Then the user could opt against either by overriding e.g.
pkgs.ledger-autosync.override { ledger = null; } or just override the dependency itself.

But we need both for the checks...

Right. OK I'll put in the flag inputs a bit later today.

Conflict with master here is 4094d4c because other prs were needing this.

Sorry to take so long ot get back on to this. I added the flags and merged with master to pick up the fuzzywuzzy derivation there. @worldofpeace

@GrahamcOfBorg build ledger-autosync

@eamsden Finished that up for you 😄

Thanks for contributing.