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

[WIP] Change inheriting valued tags to override #1955

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chrislemaire
Copy link
Contributor

@chrislemaire chrislemaire commented Dec 5, 2022

Resolves #1950

Old behaviour of inheriting tags with values was that tags added to, for instance, a posting, would be added to the tags, possibly overriding the tags of the transaction. Or, in other words, the transaction tags were added to the posting tags, as there was no sense of overriding a tag.

The new behaviour is that tags are now overridden when a lower level element re-uses that tag name. For instance, when defining a transaction with tag t:v and posting with tag t:v2, only t:v2 remains on the posting, overriding the transaction tag. The same thing is also changed for the relation between parent accounts and accounts. Although I am less sure whether that would be desired behaviour, as parent accounts might say more about child accounts than transactions do about their postings.

Perhaps it would be desirable to add an option for additive tags over overriding tags, as it may well be useful at times to add to tags in the parent?

Old behaviour of inheriting tags with values was that tags added to, for
instance, a posting, would be added to the tags, possibly overriding the
tags of the transaction. Or, in other words, the transaction tags were
added to the posting tags, as there was no sense of overriding a tag.

The new behaviour is that tags are now overridden when a lower level
re-uses that tag name. For instance, when defining a transaction with
tag t:v and posting with tag t:v2, only t:v2 remains on the posting,
overriding the transaction tag.

Perhaps it would be desirable to add an option for additive tags over
overriding tags, as it may well be useful at times to add to tags in the
parent.
Copy link

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

I think it makes sense to override in hierarchical order, meaning that the closer a tag is to the posting, the higher the precedence. This means:

  1. Tag defined for the posting
  2. Tag defined for the transaction
  3. Tag defined for the account
  4. Tag defined for the nextmost parent of the account up to the top of the account hierarchy

Another way to look at it: This order ensures that the "most specific" tag is used. Accounts are defined for the whole accounting setup and are therefore really general. Transactions are more specific as they are one instance where the account is used. The posting is the most specific.

And a third reason for this order: The closer a tag is to the posting, the higher the likelihood that the author intended the tag to be used. Tags that are defined further away from the posting (maybe even in another file) can be forgotten, so it would be weird if they would have a higher precedence.

account a:b ; atag:B

$ hledger -f- accounts tag:atag=a
a

Choose a reason for hiding this comment

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

There could be more tests for the combination of transaction/posting and account tags:

Suggested change
a
a
# 24. Transactions can override the tags of used accounts
<
account a ; atag:A
account a:b ; atag:B
2023-01-01 Test
; atag:C
(a:b) 10 €
$ hledger -f- bal --pivot=atag
10 € C
--------------------
10 €
# 25. Postings can override the tags of their accounts
<
account a ; atag:A
account a:b ; atag:B
2023-01-01 Test
; atag:C
(a:b) 10 € ; atag:D
$ hledger -f- bal --pivot=atag
10 € D
--------------------
10 €

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added your tests as suggested and I'm thinking of more tests that should be relevant to this ordering with multi-valued tags. Thank you very much for taking the time to look into some tests!

@chrislemaire chrislemaire changed the title Change inheriting valued tags to override WIP: Change inheriting valued tags to override Jan 9, 2023
@chrislemaire chrislemaire changed the title WIP: Change inheriting valued tags to override [WIP] Change inheriting valued tags to override Jan 9, 2023
@simonmichael simonmichael added needs:discussion To unblock: needs more discussion/review/exploration needs:value-proposition To unblock: needs clearer justification, review of benefits vs costs needs:testing To unblock: needs more developer testing or general usage needs:tests To unblock: needs more automated tests or test updates A-WISH Some kind of improvement request, hare-brained proposal, or plea. journal The journal file format, and its features. needs:rebase To unblock: needs to be rebased against latest master branch labels Apr 5, 2023
@simonmichael
Copy link
Owner

Here's my related comment on the parent issue #1950, suggesting things to help move this forward: #1950 (comment)

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. journal The journal file format, and its features. needs:discussion To unblock: needs more discussion/review/exploration needs:rebase To unblock: needs to be rebased against latest master branch needs:testing To unblock: needs more developer testing or general usage needs:tests To unblock: needs more automated tests or test updates needs:value-proposition To unblock: needs clearer justification, review of benefits vs costs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[💰20$ bounty] Inherited Tag Values are not Overwritten
3 participants