-
Notifications
You must be signed in to change notification settings - Fork 1
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
Minor changes to type guards, and adding tests. #124
Conversation
Are you sure vscode is using Prettier? If you press F1 and search for format, it might complain there are multiple, and then let you pick Prettier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few comments
}); | ||
}); | ||
|
||
// TODO: Testing fetchRemoteProject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to do this now, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this the long function you mentioned intentionally skipping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. It ought to be tested, but I really only touched this file to use the new parsing function, so I realized I was getting too far into the weeds; yet didn't want to delete the work I'd done already.
return true; | ||
}; | ||
|
||
export const parseSamplingOpts = (x: string | undefined): SamplingOpts => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support partial fields? My thought is if we ever expose additional arguments, old links could break if we're insisting on a full parse.
I know we don't currently, so it's fine to not do this now, just speculating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a good question. So far we appear to be insisting on it, but fortunately for now the only way to get one of these (other than to craft it manually, which, why) would be to save one, and as it currently stands that will give a full object.
...but going forward, yeah, we should probably drop in defaults for non-provided values.
This implements the changes suggested in #93 (too late for inclusion in that PR).
Specifically, I've added a minor utility function to reduce the repeated basic checks in the type guards, as well as implementing more detailed deserialization-time validity checking for the sampling options.
I've also added some (pedantic, not very flexible) tests for the type guards implemented in that PR, as well as a sketch of some for the query parameter loading file (not complete coverage, since I left out the long function which was--hopefully--out of scope for testing with the rest of the changes I was making).
I've also reorganized a little bit to move some utility functions into common areas, and to (somewhat) reduce the fragmentation of types & type guards across the code (by moving the sampling options definition into the
ProjectDataModel
file--though perhaps there's a better place for these?)I do not know why my
Prettier
format-on-save insists on resetting to 4 spaces for imports...