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

ledger-autosync: init at 1.0.0 #53261

Merged
merged 2 commits into from Feb 6, 2019
Merged

Conversation

@eamsden
Copy link
Contributor

@eamsden eamsden commented Jan 3, 2019

Motivation for this change

ledger-autosync is a useful utility for ledger and hledger which allows journal files to be synchronized with OFX and CSV transaction data without duplication.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 3, 2019

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

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 3, 2019

@worldofpeace

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

Force push or new pr?

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 3, 2019

@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.

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 4, 2019

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.

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 5, 2019

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

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 5, 2019

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.

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 6, 2019

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)

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 6, 2019

@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.

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 6, 2019

@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?

# Checks require ledger as a python package,
# ledger does not support python3 while ledger-autosync requires it.
checkInputs = with python3Packages; [ ledger hledger nose mock ];
checkPhase = ''
Copy link
Contributor

@worldofpeace worldofpeace Jan 6, 2019

Choose a reason for hiding this comment

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

Can we get a comment here also stating that we've omitted the ledger-python tests because they're broken?
It's for the same reason stated above but it's not obvious we're omitting a test here.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 6, 2019

@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.

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 10, 2019

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?

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 10, 2019

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;
  }) 
];

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 10, 2019

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.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 11, 2019

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...

@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 12, 2019

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.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Jan 25, 2019

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

@eamsden eamsden changed the title Add ledger-autosync and fuzzywuzzy python package ledger-autosync: init at 1.0.0 Jan 28, 2019
@eamsden
Copy link
Contributor Author

@eamsden eamsden commented Jan 28, 2019

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

@worldofpeace worldofpeace force-pushed the eamsden-ledger_autosync branch from e31e909 to 92713b4 Feb 6, 2019
@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Feb 6, 2019

@GrahamcOfBorg build ledger-autosync

@worldofpeace worldofpeace merged commit 42bdc36 into NixOS:master Feb 6, 2019
2 of 5 checks passed
@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Feb 6, 2019

@eamsden Finished that up for you 😄

Thanks for contributing.

@eamsden eamsden deleted the eamsden-ledger_autosync branch Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants