hledger icon indicating copy to clipboard operation
hledger copied to clipboard

feat: register: add --sort as in ledger

Open reesmichael1 opened this issue 1 year ago • 7 comments

This begins work towards implementing the --sort argument for register, as requested in #2180. This still needs some polishing (e.g., documentation), but I wanted to get this up to discuss the implementation. The biggest question to me is if SortSpec is better placed in ReportOptions, which would let it be reused across the other reports along with just register (at the very least, it seems like this should be reused across aregister).

Moving it would also let us calculate it internally in postingsReport instead of awkwardly passing in a Maybe SortSpec wherever postingsReport is called.

reesmichael1 avatar Jul 16 '24 03:07 reesmichael1

Looking good!

Now that I've played with it a bit more - I think people will want description sorting as well. Could we accept that, and also desc, as that spelling is accepted elsewhere.

More ideas, though not blockers:

  • Somebody will probably want absamount sooner or later ? (or: YAGNI)
  • It would be great to add this also to aregister. (for consistency)

simonmichael avatar Jul 22 '24 16:07 simonmichael

I didn't test this, but I wondered: does a date or -date sort preserve parse order when sorting postings on the same date ? Hopefully yes.

simonmichael avatar Jul 22 '24 16:07 simonmichael

Thanks! I actually started adding it to aregister but stopped for some reason I can't remember, so I'll try to wire it in and see what it's actually like. I think it was probably because I thought the balance total column would be weird, but on more reflection, I think having it for transactions there would be useful.

I'll add in the description/desc as well! I'll see about absamount while I'm working on it.

I'll verify again, but I'm pretty sure the date and -date sorting does indeed preserve order within the same date as it should.

reesmichael1 avatar Jul 22 '24 19:07 reesmichael1

Ok, I think I've hacked together a way to get abs working for MixedAmount in 2d45393, along with adding desc/description. I'll look at aregister next, although it might be a day or two before I have time to wrap that up.

reesmichael1 avatar Jul 23 '24 02:07 reesmichael1

I'd hoped to get aregister working before leaving on vacation, but getting ready ended up taking all of my time (I'm typing this from a layover!). It'll be a few weeks before I have a chance to get back to aregister, so if you're happy with this as is, feel free to merge it now, or I can continue when I get back!

reesmichael1 avatar Jul 31 '24 14:07 reesmichael1

Thanks @reesmichael1. I'd be ok with merging this now but there's one niggling change needed I think: also mention the allowed --sort keywords in 1. the option's help and 2. the error message when a bad value is given (and ideally also when no value is given, but I suspect that's too hard). No hurry on this, vacation++ 🏝️

simonmichael avatar Aug 04 '24 18:08 simonmichael

PS: I rebased.

simonmichael avatar Aug 04 '24 18:08 simonmichael

I have merged this to catch the next release. Thanks a lot @reesmichael1! If/when you work on it again (for aregister), this is still worth considering:

mention the allowed --sort keywords in the option's help, and in the error message when a bad value is given, (and ideally also when no value is given, but I suspect that's too hard)

simonmichael avatar Sep 05 '24 11:09 simonmichael

Thanks for taking over for me! Sorry for disappearing, I have been swamped with life over the last several weeks. I'll try to get back to continuing this soon!

reesmichael1 avatar Sep 16 '24 19:09 reesmichael1