-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add ability to do typechecking and conversion on the values of key-value options #1032
Conversation
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.
Would it not be much simpler to make the KeyValueType<T>
a class and then create objects with new instead of using KeyValueTypeDefinition
?
You don't need new instances each time they are used. The definitions are the same, so only one object needs to be used for each type. (Except the If you have something else in mind, can you be more explicit about it? |
I did not mean to create a new object in every call. I was more thinking about having a single class instead of a type and definition function, i.e., something along the line of: export class KeyValueType<T> {
constructor(
public name: string,
public verify: (value: string) => boolean,
public convert: (value: string) => T
) {}
} and then rewriting the export const KeyValueTypes: {[name: string]: KeyValueType<any> | ((data: any) => KeyValueType<any>)} = {
boolean: new KeyValueType<boolean>(
'boolean',
(value) => value === 'true' || value === 'false',
(value) => value === 'true'
),
... This way we still only create one instance object for each entry. But we save the |
OK, I made the changes from the object to a class. That is a good change, thanks. Please take a look. |
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.
lgtm.
This PR adds optional type checking and conversion to the
keyvalueOptions()
function. Currently, theallowed
argument gives an object that says what keys are allowed (ones with non-zero values in that object). Here, we extend the use of theallowed
object to provide data for type checking and converting the values of the given keys.This is done through a new
KeyValueType
object that hasvalidate()
andconvert()
methods (and a name, for potential error reporting). There is aKeyValueTypeDefinition()
function that makes creating these objects easier, and aKeyValueTypes
object that holds predefined definitions for the standard types (boolean, number, string) and some variations (integer, dimen, andoneof()
that gives a collection of valid option for the string). These seem to be a good base set of definitions, but you can create your own custom types with more detailed checker and converter, as needed.We aren't currently using this feature now, but it would be useful in the siunitx extension that I am reviewing for PCC now.
A similar idea may also be useful for making the option checking more robust in the future (in
util/Options.ts
), where we only do key name checking, but not type checking currently.