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

Parse partial sampling opts, fill remaining keys with defaults #147

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

WardBrian
Copy link
Collaborator

Closes #125

@WardBrian WardBrian requested a review from jsoules July 24, 2024 19:18
Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

Code changes are modest and look good to me. Tested by saving a project, deleting some keys, and reloading, which confirmed the deleted keys were replaced with defaults.

@@ -33,13 +33,16 @@ const validateSamplingOpts = (x: SamplingOpts): boolean => {
if (naturalFields.some((f) => !Number.isInteger(f))) return false;
if (positiveFields.some((f) => f <= 0)) return false;
if (nonnegativeFields.some((f) => f < 0)) return false;

if (Object.keys(x).length !== 5) return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I love hard-coding the 5; it isn't likely to be an issue in a serious way but I can see us tripping over this every time we change the number of potential sampling opts (which admittedly isn't going to be often).

@WardBrian WardBrian merged commit 94a634f into main Jul 24, 2024
2 checks passed
@WardBrian WardBrian deleted the parse-partial-sampling-opts branch July 24, 2024 19:53
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.

Allow partial parse for SamplingOpts
2 participants