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

Restore Settings Validation (Splink 4) #2127

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

ADBond
Copy link
Contributor

@ADBond ADBond commented Apr 3, 2024

Previously while work on Settings/SettingsCreator etc were occurring the settings validation code was unhooked.

This restores that code, while deleting the portions that I don't think are relevant, now that we are not dealing with dictionaries (by the time the Settings is instantiated).

It may well make sense to rewrite some of this code ultimately to fit more naturally in line with the new way we are constructing things, but for now it seems reasonable to restore the functionality we had, before perhaps moving the logic around.

Feel free to disagree with what I've removed/whether this makes sense overall.

@ADBond ADBond force-pushed the settings-validation-splink4 branch 2 times, most recently from 00d2d95 to 85ea454 Compare April 3, 2024 06:25
@ADBond ADBond force-pushed the settings-validation-splink4 branch from 85ea454 to abaa23b Compare April 3, 2024 06:29
@ADBond ADBond changed the title [WIP] Restore Settings Validation (Splink 4) Restore Settings Validation (Splink 4) Apr 4, 2024
@ADBond ADBond requested a review from RobinL April 4, 2024 01:01
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.

Great, thanks. There's a small linting issue but happy to merge once fixed

@ADBond
Copy link
Contributor Author

ADBond commented Apr 4, 2024

The linting issue was fixed by the autoblack commit - couldn't figure out a way to easily re-trigger CI checks unfortunately!

@ADBond ADBond merged commit ea29382 into splink4_dev Apr 4, 2024
@ADBond ADBond deleted the settings-validation-splink4 branch April 4, 2024 05:48
@RobinL
Copy link
Member

RobinL commented Apr 4, 2024

Oh yeah - of course. Thanks

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