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

Adding type-hints, part 1 #2169

Merged
merged 23 commits into from
May 8, 2024
Merged

Adding type-hints, part 1 #2169

merged 23 commits into from
May 8, 2024

Conversation

ADBond
Copy link
Contributor

@ADBond ADBond commented May 8, 2024

Mostly adding a bunch of type hints to the code base.

This gets rid of approximately half of the mypy errors we get if we include the flag disallow_incomplete_defs, and thus is a stepping stone to include that check in our config.

The only noteworthy changes:

  • Added a non-None default for estimate_u_with_random_sampling, as per linker.estimate_u_using_random_sampling fails with default arguments, with no clear indication why #2165 - needed some solution to avoid breaking mypy checks, but we can change this again in follow-ups
  • ComparisonLevel has a (read-only) property comparison_vector_value - this is just a wrapper around the internal _comparison_vector_value, that does not allow the None option we have before it is associated with Comparison. This is mostly what we want to use anyhow

Copy link
Member

@RobinL RobinL left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry for the merge problem, was just about to fix it but you've done it!

@ADBond ADBond merged commit 68ec673 into splink4_dev May 8, 2024
14 checks passed
@ADBond ADBond deleted the th1 branch May 8, 2024 19:00
@ADBond
Copy link
Contributor Author

ADBond commented May 8, 2024

Thanks! Sorry for the merge problem, was just about to fix it but you've done it!

No worries - your PRs have been there a while so happy to sort out the merge - nothing major anyhow. Cheers!

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

Successfully merging this pull request may close these issues.

2 participants