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

lib: Fix filtering by payee and note (#598) #608

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@zarybnicky

zarybnicky commented Aug 30, 2017

Fixes #598.

Neither parseQuery nor matchPosting/matchTransaction were set up to accept payee:/note: filters. Also, transactionNote was non-functional (it returned payee) and payeeAndNoteFromDescription didn't actually fulfill its documentation, namely "When there's no |, payee == note == description."

Edit: It's also occurred to me that in the Query datatype, Desc and Code are obsolete. I'd suggest either replacing them with either Tag "desc", or maybe with an ImplicitTag type.

Edit2: After looking at the code in more detail (and fixing broken tests, namely not:tag:.), it seems to me that the whole Query module could use some more datatypes instead of string matching - eg. aggregate implicit tags into a data ImplicitTag (with Desc and Code there as well) or using a parser instead of parseQueryTerm with a cascade of T.stripPrefix.

@zarybnicky

This comment has been minimized.

Show comment
Hide comment
@zarybnicky

zarybnicky Aug 30, 2017

Travis says: Invalid symmetric difference expression master...2292800f4224c95525e540e0cdb6c20af0a7a9b6
Let me try again...

zarybnicky commented Aug 30, 2017

Travis says: Invalid symmetric difference expression master...2292800f4224c95525e540e0cdb6c20af0a7a9b6
Let me try again...

@zarybnicky zarybnicky closed this Aug 30, 2017

@zarybnicky zarybnicky deleted the zarybnicky:filter-payee branch Aug 30, 2017

@zarybnicky zarybnicky restored the zarybnicky:filter-payee branch Aug 30, 2017

@zarybnicky zarybnicky reopened this Aug 30, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Sep 5, 2017

Owner

Thanks for the fixes! Actually I hadn't thought of making payee and note searchable like first-class fields (tag:payee=... is what I expected to work). But after thinking about it, it makes sense. I think we should just say "you can pivot on these field names (description, payee, note..) or any tag name", and drop the "implicit tags" concept.

Aside: the payee/note feature is a complication, but I've wanted it often enough that I think it's worth keeping. I've been brainstorming different syntax/semantics for it, but I think the current behaviour is reasonable. We should probably add some tests. I will update docs soon.

Owner

simonmichael commented Sep 5, 2017

Thanks for the fixes! Actually I hadn't thought of making payee and note searchable like first-class fields (tag:payee=... is what I expected to work). But after thinking about it, it makes sense. I think we should just say "you can pivot on these field names (description, payee, note..) or any tag name", and drop the "implicit tags" concept.

Aside: the payee/note feature is a complication, but I've wanted it often enough that I think it's worth keeping. I've been brainstorming different syntax/semantics for it, but I think the current behaviour is reasonable. We should probably add some tests. I will update docs soon.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Sep 5, 2017

Owner

Rebased and merged as 72cf6a8.

Owner

simonmichael commented Sep 5, 2017

Rebased and merged as 72cf6a8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment