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

Namespaces #780

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Namespaces #780

wants to merge 2 commits into from

Conversation

lars-reimann
Copy link
Collaborator

What problem do you want to solve?

Reference the issue or discussion, if there is any. Provide a description and/or bullet
points to describe the changes in this PR.

Todo

  • Pick an appropriate title.
  • Put Closes #XXXX in the first PR comment to auto-close the relevant issue once
    the PR is accepted. This is not applicable if there is no corresponding issue.
  • Document PR in CHANGES.md.

@lars-reimann
Copy link
Collaborator Author

lars-reimann commented Jul 17, 2024

@MImmesberger The nested function dictionary is currently structured like this:

image

Which levels of nesting should be removed? Based on our previous discussion, it's probably the first two, i.e. _gettsim and social_insurance_contributions/transfers/taxes/demographic_vars, right?

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.31%. Comparing base (ffbccf3) to head (c550526).

Files Patch % Lines
src/_gettsim/functions_loader.py 0.00% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #780      +/-   ##
==========================================
- Coverage   89.69%   89.31%   -0.39%     
==========================================
  Files          53       53              
  Lines        3796     3810      +14     
==========================================
- Hits         3405     3403       -2     
- Misses        391      407      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MImmesberger
Copy link
Collaborator

I think that, for example, transfers.arbeitsl_geld.betrag is perfect, so I would vote for keeping the social_insurance_contributions/transfers/taxes/demographic_vars level. The _gettsim should not be part of the target call.

@hmgaudecker
Copy link
Collaborator

hmgaudecker commented Jul 17, 2024

Looking at that, I'd almost think that transfers and taxes are superfluous, but not demographics (I'd suggest renaming this) and social_insurance_contributions, although we might be able to come up with a shorter name.

What do you think, @MImmesberger ?

In any case, not super-important for the moment, the good thing is that it will be fairly easy to do bulk-renamings / removals (granted it seems more difficult to insert a level back than to remove it, so I'd be fine with an approach applying a bit more caution at the moment).

@MImmesberger
Copy link
Collaborator

I agree, there should be no overlap between the elements one level below the tax/transfer level, so no need to explicitly distinguish. In that case, however, we should make sure that the module docstring what the module is about (e.g. it's not obvious whether the "Kinderbonus" is a tax deduction or a transfer).

Regarding the naming of social_insurance_contributions: In the google doc, I called this "sozialversicherungsbeitraege". But I just realized that I put this in the taxes namespace which is wrong. Should be its own (as it is the case currently in Lars' proposal).

@hmgaudecker
Copy link
Collaborator

Agreed, let's use sozialversicherungsbeitraege. At some point we might rename demographics yet again, but that really should be a simple Search & Replace.

I'm all for good docstrings, but any tax deduction should go into the tax component it is deducted from, right?

@MImmesberger
Copy link
Collaborator

I'm all for good docstrings, but any tax deduction should go into the tax component it is deducted from, right?

Yes definitely. Was just thinking about making it as obvious as possible. In our case "Kinderbonus" is a transfer.

@ChristianZimpelmann
Copy link
Member

sozialversicherungsbeitraege

More in line with the naming of modules might be sozialv_beitraege

@hmgaudecker
Copy link
Collaborator

Good catch! Those are scheduled to be changed to

  • arbeitslosenversicherung
  • einkommensgrenzen
  • krankenversicherung
  • pflegeversicherung
  • rentenversicherung

But that was not impossible to know, ofc!

@lars-reimann
Copy link
Collaborator Author

lars-reimann commented Jul 17, 2024

Should the renamings/removals be done programmatically in the new function or by changing the directory structure under _gettsim?

Edit: For now, remove transfers/taxes in the new function. Later, change the directory structure, so it matches the dictionary.

@hmgaudecker
Copy link
Collaborator

Just came across #533 -- might that be fixed in passing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants