Skip to content

Conversation

@lars-reimann
Copy link
Contributor

@lars-reimann lars-reimann commented Apr 30, 2024

What problem do you want to solve?

Closes #740

join_numpy now takes a value that is used if a foreign key cannot be resolved, e.g. because it is explicitly set to -1.

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 lars-reimann linked an issue Apr 30, 2024 that may be closed by this pull request
@lars-reimann lars-reimann changed the title fix: fall back to default value if foreign key is unresolved fix: fall back to default value if foreign key of join is unresolved Apr 30, 2024
@lars-reimann lars-reimann marked this pull request as ready for review April 30, 2024 15:28
@lars-reimann
Copy link
Contributor Author

lars-reimann commented Apr 30, 2024

  1. Currently, a default value must always be specified. Would that suffice for now?
  2. Should there still be a check of the validity of foreign keys? I've kept it for non-negative foreign keys.
  3. A default value for _unterhaltsvorschuss_empf_eink_above_income_threshold must still be added.

@hmgaudecker
Copy link
Collaborator

  1. I think the name "default" is misleading a bit. Maybe: "value_if_foreign_key_is_missing"? Too long, maybe... But yes, we should be explicit there.
  2. Validity checks for non-negative numbers sound good.
  3. That'll be up to @MImmesberger

Thanks!!!

@lars-reimann lars-reimann force-pushed the 740-bug-join_numpy-return-if-foreign-key-is-0 branch from 736bf7b to c369028 Compare April 30, 2024 16:39
@lars-reimann lars-reimann changed the title fix: fall back to default value if foreign key of join is unresolved fix: fall back to default value if foreign key of join_numpy is unresolved Apr 30, 2024
@lars-reimann lars-reimann force-pushed the 740-bug-join_numpy-return-if-foreign-key-is-0 branch from c369028 to 867d6a8 Compare April 30, 2024 16:44
Copy link
Collaborator

@MImmesberger MImmesberger left a comment

Choose a reason for hiding this comment

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

Thanks, that was fast!
I tried it out on the branch of #739 and it works as expected. Regarding the naming: I think value_if_foreign_key_is_missing makes it clearer what is happening for the average user than value_for_unresolved_foreign_key.

@MImmesberger
Copy link
Collaborator

I just renamed the new argument and adjusted the join for Unterhaltsvorschuss accordingly.

@codecov
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.41%. Comparing base (23e7b4f) to head (42c5eca).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #741   +/-   ##
=======================================
  Coverage   89.41%   89.41%           
=======================================
  Files          51       51           
  Lines        3646     3647    +1     
=======================================
+ Hits         3260     3261    +1     
  Misses        386      386           

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

Copy link
Collaborator

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Great, thanks! Approval conditional on fixed tests, ofc. (feel free to merge if they'll be fixed immediately in another PR).

@MImmesberger MImmesberger merged commit 6c234b2 into main Apr 30, 2024
@MImmesberger MImmesberger deleted the 740-bug-join_numpy-return-if-foreign-key-is-0 branch April 30, 2024 17:41
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.

BUG: join_numpy return if foreign key is < 0

4 participants