-
Notifications
You must be signed in to change notification settings - Fork 31
Change namespace of private Renteneinnahmen #1029
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
Change namespace of private Renteneinnahmen #1029
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! Will make it much clearer, I'd like to suggest some renamings, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks! Just some tiny adjustments, feel free to disagree though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer removing this function. It has nothing to do with einkommensteuer and having einnahmen underneath the einkünfte namespace is a bit confusing. I'd just use the three components as arguments to the three functions where it is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm indifferent, but I feel like we should make a general rule out of this to make it easier for future developers.
What do you think about a "GETTSIM style guide", i.e. conventions that are not strictly required but an ideal we aim for? I feel like there is a growing informal knowledge between the two of us that others will struggle to pick up.
Obv nothing that should be done right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having
einnahmenunderneath theeinkünftenamespace is a bit confusing
This speaks for adding an einnahmen namespace and put all the policy_inputs of einkünfte in there. Those are all einnahmen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that would be a major change, which would seem conceptually very helpful though:
root
└── einnahmen
├── aus_nichtselbstständiger_arbeit
| └── bruttolohn_m
├── renten
| ├── gesetzliche_m(sozialversicherung...altersrente, sozialversicherung...erwerbsminderungsrente)
| ├── betriebliche_altersvorsorge_m
| ├── geförderte_private_vorsorge_m
| └── sonstige_private_vorsorge_m
⋮
└── aus_forst_und_landwirtschaft_y
└── einkommensteuer
├── einkünfte
| ├── aus_nichtselbstständiger_arbeit
| ├── betrag_y(einnahmen__aus_nichtselbstständiger_arbeit__bruttolohn_y)
⋮
| └── I feel like we should make a general rule out of this to make it easier for future developers.
| ├── betrag_y(einnahmen__aus_forst_und_landwirtschaft_y)
⋮
That is, the einnahmen only follow EStG in the classification, but they are otherwise detached from it and we may have things that are actually different.
Conceptually, in most places einnahmen__gesetzliche_m is relevant as opposed to the fact that it is in the sozialversicherung namespace. Would also make it clearer for people who want to override this.
Looking at it, we probably would leave out the einnahmen___aus_forst_und_landwirtschaft_y since we don't model the deductions, right?
I feel like we should make a general rule out of this to make it easier for future developers.
Not sure there can be a general rule in terms of "always sum up three values" or not. Maybe I'll have a different opinion on this one after the above change, too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you add things like income from ALG2, Elterngeld etc. to this namespace?
The big upside of the current structure is that we adhere very closely to the law. If we have the top-level einnahmen namespace it becomes apparent what we mean with bruttolohn_m only when looking at the docstring.
However, if we add all kinds of income there (not only EStG relevant), this would end the struggle of many users to find the correct nodes of the TT DAG.
I tend towards the pros outweighing the cons.
Not sure there can be a general rule in terms of "always sum up three values" or not.
I'm thinking more about ordering of function arguments, ordering of functions inside the modules, naming conventions (Einnahmen/Einkünfte,...), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure there can be a general rule in terms of "always sum up three values" or not.
I'm thinking more about ordering of function arguments, ordering of functions inside the modules, naming conventions (Einnahmen/Einkünfte,...), etc.
Ah, then maybe I was confused by the location of the comment... 😉 But yeah, will be useful to document somehow. Sounds like a job for an AI to tell us which rules we have been using predominantly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you add things like income from ALG2, Elterngeld etc. to this namespace?
The big upside of the current structure is that we adhere very closely to the law. If we have the top-level
einnahmennamespace it becomes apparent what we mean withbruttolohn_monly when looking at the docstring.However, if we add all kinds of income there (not only EStG relevant), this would end the struggle of many users to find the correct nodes of the TT DAG.
I tend towards the pros outweighing the cons.
In the end, it is that a labels-based structure would be even better than our hierarchical structure. (I would not know how to do that, though). Same with wohnen, familie, ... We would love to give these things a bunch of labels, for whatever they are relevant. Instead, we need to file them in one drawer.
I guess the einnahmen are pretty close to things like familie -- i.e., state variables that are fairly independent of details of the taxes and transfers system. bruttolohn_[x] is a pretty good case in point. I count 38 occurrences in function interfaces outside of the einkommensteuer namespace...
Similar to the rente__gesetzliche_m suggestion above, we could think about copying over other sources of income. Though that might yield some room for confusion, so I'd do that only later.
src/_gettsim/einkommensteuer/einkünfte/sonstige/rente/rente.yaml
Outdated
Show resolved
Hide resolved
|
Will merge; we should tackle moving the namespaces in a new PR. |
3a8e894
into
collect-components-of-namespaces
What problem do you want to solve?
Closes #893
Changes:
Issue for historical support of TT rules: #1030