search

hpdeifel / hledger-iadd

A terminal UI as drop-in replacement for hledger add.
BSD 3-Clause "New" or "Revised" License
84 stars 8 forks source link

Add a default currency option #6

Closed trishume closed 8 years ago

trishume commented 8 years ago

Add a flag and config variable that contains a string that is prepended to amounts when they are just bare decimal numbers with no other currency. Useful for people that use only one currency most of the time, which is probably most people.

I threaded it all the way through just like the date format, but I don't think that threading approach is nice or scalable, the diff for adding a config option like this shouldn't be as large as this PR is. See the first commit (which I'll squash if you want) for how easy it was without the config option.

@hpdeifel

hpdeifel commented 8 years ago

Is this really needed? hledger already supports a default commodity that's specified in the journal. E.g I have

D 1.000,00€

somewhere in there. Since hledger-iadd uses hledger-lib to parse amounts, this is already supported and even shown in the completion. If I type "5" it completes to "5,00€".

As you see it's also not as simple as prepending the currency, since many currencies are written after the amount.

You're right about the code design though. I like the leaf functions to only receive the inputs they really need, since that makes interactive and automated testing a lot easier. But for the middle layer (such as nextStep, context and the like), a single record parameter or a reader monad would probably be a good idea.

trishume commented 8 years ago

Whoah I wish I had known about this before I spent a bunch of time coding this feature. Derpaderpderp.

I was wondering why such an obviously important feature wasn't included by default 😛

trishume commented 8 years ago

Well if you or I ever want to thread one additional config variable through (maybe fuzzy matching), you can just take this branch and find/replace. Won't work for the one after that though.

hpdeifel commented 8 years ago

Yeah, sorry for the wasted effort.

I had the default commodity set for a long time and kinda took it for granted. Didn't even occur to me to add it to the README