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

Add payees command #1063

Merged
merged 7 commits into from
Jul 15, 2019
Merged

Add payees command #1063

merged 7 commits into from
Jul 15, 2019

Conversation

alerque
Copy link
Collaborator

@alerque alerque commented Jul 10, 2019

This stuff is to fix #1062. Feel free to contribute ;-)

@alerque alerque force-pushed the payees-command branch 2 times, most recently from fbabcdd to 202c922 Compare July 10, 2019 08:43
@simonmichael
Copy link
Owner

simonmichael commented Jul 10, 2019 via email

@alerque
Copy link
Collaborator Author

alerque commented Jul 10, 2019

Good question. I started with payees as the command because I was thinking of ledger feature parity more than anything else, but as long as an equivalent feature exists and we're not worried about matching usage syntax, maybe descriptions with options for just payee or just note output would be better.

@alerque
Copy link
Collaborator Author

alerque commented Jul 10, 2019

Thanks for the pointers getting the basic bits working. As discussed in IRC I'm splitting this into three commands, but I'd be happy to take it back in the direction of a single command with flags if that's the consensus.

@alerque alerque force-pushed the payees-command branch 2 times, most recently from f19633a to 8b31d5b Compare July 10, 2019 11:26
@alerque alerque marked this pull request as ready for review July 10, 2019 11:26
@alerque alerque force-pushed the payees-command branch 2 times, most recently from 2284cbb to f4bd3e2 Compare July 10, 2019 11:51
@simonmichael
Copy link
Owner

Looking good. Here's an attempt at making command help more informative:

 descriptions             show unique transaction descriptions
 notes                    show the unique note parts of transaction descriptions
 payees                   show the unique payee parts of transaction descriptions

Descriptions which don't explicitly define a note and payee with a | character, are included in the output of all three commands. This is correct according to hledger's internal semantics; do you think it would be more practical and intuitive if notes and payees showed only the explicitly defined fields ?

@alerque
Copy link
Collaborator Author

alerque commented Jul 10, 2019

Yes, I think it would be more intuitive if empty fields were not cross pollinated from data explicitly recorded as something else. I need to think about this more. I instinctively said yes, but I looked at my actual previous use cases and at least one major one is well serviced by returning the payee when the note field is empty.

@simonmichael
Copy link
Owner

PS and a larger question, is this whole concept (optionally splitting description into payee and note with a pipe character) reasonable and worth the trouble ? I haven't used it for real yet myself, and haven't heard of anyone using it.

@alerque
Copy link
Collaborator Author

alerque commented Jul 10, 2019

I'm using it for several things in production and quite like the flexibility.

@simonmichael
Copy link
Owner

On IRC we discussed making notes and payee show only explicitly-defined notes and payees, excluding descriptions which don't contain a pipe character. Some real-world testing will probably tell if that's practical. If not, maybe an option to include them ? --all ?

@simonmichael simonmichael added the A-WISH Some kind of improvement request, hare-brained proposal, or plea. label Jul 10, 2019
@simonmichael
Copy link
Owner

simonmichael commented Jul 10, 2019

returning the payee when the note field is empty

I saw your edit above. Strictly speaking, neither note or payee are ever empty (unless the whole description is empty). If they aren't marked explicitly with a |, both of them take the whole description as their value. [So in that case, description == payee == note.]

@simonmichael
Copy link
Owner

Your more i18n-robust and case-insensitive sorting seems like a nice feature, perhaps to be added to all commands in due course. I believe text-icu requires a corresponding C lib to be installed, so we'll need to find out how it affects installability. Here are some possibly related windows issues.

@alerque
Copy link
Collaborator Author

alerque commented Jul 10, 2019

The major use case I've been using it for is for the case of a broker vendor that is constantly billing my credit card, but the actual vendor fulfilling me with product is different every time. I specifically want to track both that the transactions are through a broker and the actual vendor. In fact sometimes I buy things from the same vendors directly, not through the broker.

An analogous case for some people in the US might be eBay (do people still use that over there?). Anyway in this case the payee might be eBay, but you are also buying from a certain vendor and might want to sort your transactions based on the seller too.

2019/07/10 eBay | Seller A
  abc

2019/07/10 eBay | Seller B
  abc

2019/07/10 Seller A
  abc

For this (very real world for me, I have my personal finances and on org worked up this way) use case the current behavior of conflating the fields when only one of them is filled in is actually the preferable usage.

@simonmichael
Copy link
Owner

simonmichael commented Jul 10, 2019

(Also, text-icu is currently unmaintained). [Though it is in stackage, which is something.]

@alerque
Copy link
Collaborator Author

alerque commented Jul 10, 2019

@simonmichael That doesn't look very positive. I don't know much about the Haskell ecosystem or how big an issue that is — particularly how likely it is to just stop working in conjunction with some other library being used.

@simonmichael
Copy link
Owner

It seems like text-icu is used enough that it's unlikely to stop working (for very long).. and has kept working well enough that no new maintainer has stepped up. I wouldn't rule out depending on it, but only if we can satisfy ourselves that install instructions on the download page can still be fairly short.

@simonmichael simonmichael added needs:testing To unblock: needs more developer testing or general usage packaging Dependencies, version constraints, packaging.. labels Jul 10, 2019
@the-solipsist
Copy link
Collaborator

You'll need to add to the documentation what happens if there are multiple | characters in a description. What part is take as payee (all the text until the last |, or only the text until the first |?), and what part is taken as note.

@alerque
Copy link
Collaborator Author

alerque commented Jul 12, 2019

@the-solipsist That's not particularly relevant to this PR because the added commands don't do any of the parsing or storing of this data internally, they just dump what is there according to hledger's existing parsing behaviour. That being said if a clarification is needed I'm sure something could go in the journal format docs. And based on a quick empirical test it looks like payee is everything up to the first pipe, and note is everything after the first pipe, including later pipes and following content if present.

@the-solipsist
Copy link
Collaborator

That's not particularly relevant to this PR

You're absolutely right. I'm sorry, I read this comment too quickly, and thought it was documentation/manpage, rather than the --help output. This belongs in the documentation/manpages, and not in the --help output.

@alerque
Copy link
Collaborator Author

alerque commented Jul 12, 2019

@simonmichael I added your roughly your suggestion for longer help texts.

@the-solipsist I also expanded the journal format documentation just a bit to cover that question.

@simonmichael
Copy link
Owner

@alerque not sure if you saw my chat message - how about moving the text-icu sorting commit to a separate PR ?

@alerque
Copy link
Collaborator Author

alerque commented Jul 14, 2019

@simonmichael Nope, sorry I'm not actually following along 100% in chat. But sure that sounds reasonable if the ICU addition is what is holding up merging this. I just extracted that commit for now.

BTW your commit message syntax is ... kind of opaque to me. Is your convention documented somewhere?

@simonmichael simonmichael removed the needs:testing To unblock: needs more developer testing or general usage label Jul 15, 2019
@simonmichael
Copy link
Owner

I missed the latest force push. Thank you!

@simonmichael simonmichael merged commit 7e332fd into simonmichael:master Jul 15, 2019
@alerque alerque deleted the payees-command branch July 15, 2019 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. packaging Dependencies, version constraints, packaging..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add payees command to list all used payees
3 participants