Improve `validate()` function for plugin options #1323

JeffersGlass · 2023-03-29T02:02:33Z

This improves the previous validate() function in pyterminal.ts to be more flexible in how it handles potential values and default values. Previously, configuration options could only be booleans or strings, and the default had to be a string.

Now, the options can be any of the non-array types (string, boolean, number or Object) that could be present in the TOML/JSON from py-config, any of them can be the default, and the type system will catch whether the listed default value is indeed part of the array.

I've moved validate() - now renamed to validateConfigParameter() - from pyterminal.ts to plugin.ts, but only used it to improve pyterminal.ts for now to make it easier to see what's changed. We can use it to validate parameters for the other plugins in follow-up PRs. I've also added validateConfigParameterFromArray(), which handles the (currently) common situation where the valid values are drawn from a small list known ahead of time.

~~This could probably now use a test or two, hence the WIP~~. ~~Tests written; this is ready for review.~~ ~~Following Antonio's thoughts about broadening what this could do, I'm reverting this to draft for now.~~ This is ready for review again.

I also want to use this functionality in #1317 (xterm.js), so I'm prioritizing this at this moment.

antocuni

+1 for moving the validation logic to plugin.ts and have a robust function which all the plugins can use.

-0 for making it overly complicated w.r.t. types.
If I understand correctly, a good chunk of the logic is needed to make the typescript compiler happy. My question is: is this extra complexity worth it?

Also, I can see how this works if you have a fixed set of options: you just pass all the possible values. But what if I want to specify a generic string?
Or a number?
An integer?
An integer between -4 and 102?

Note that I'm not saying that you should implement all the logic in this PR, but it's probably a good idea to plan ahead and think what are the functionalities that we want to support, because it might change the way we design the validation logic.

pyscriptjs/src/plugins/pyterminal.ts

JeffersGlass · 2023-03-29T14:41:23Z

If I understand correctly, a good chunk of the logic is needed to make the typescript compiler happy. My question is: is this extra complexity worth it?

It also reduces complexity in unit tests - once the type system guarantees that default will be a member of possible_values for every configuration option, we can test checkConfigOption essentially once. Otherwise, I think we'd need a test to ensure that the default for terminal_options is valid, another that the default for docked_settings is valid, etc, because there'd be nothing preventing someone down the road from writing:

checkedConfigOption({ possible_values: [true, false, 'auto'], default: 'BANANAS' });

other than the integration tests that involve those individual attributes, which may or may not consistently handle 'invalid' options themselves.

Also, I can see how this works if you have a fixed set of options: you just pass all the possible values. But what if I want to specify a generic string? Or a number? An integer? An integer between -4 and 102?

This is a great thought, and not something I'd considered... let me muse on it and see if there's a straightforward line to something that would allow that.

JeffersGlass · 2023-03-29T20:04:47Z

I've got a promising start an a change where a user could pass their own validation function. Needs some more API thoughts but the bones are there. Reverting this to draft for now until it's ready... probably will want a re-review when that's complete.

JeffersGlass · 2023-04-03T16:02:19Z

@antocuni this is ready for a re-review, when you have a chance.

validateConfigParameter is now much more general - it accepts a validator function which it uses to determine whether the user-supplied value is valid, which should allow for a lot of flexibility down the road, like the cases you listed.

validateConfigParameterFromArray is a helper method which will be broadly applicable to the parameters we use at the moment, for situations where the valid values are drawn from a small list we know ahead of time.

One can imagine adding more helper methods down the road - validateConfigParameterIsString, validateConfigParameterIsPositiveNumber, etc, , but for now I've just written the one needed for pyterminal.

~~test_pyrepl_exec_hooks is failing intermittently, but that's unrelated. Still sad though. ~~ #1363 fixed the issue! Huzzah!

for more information, see https://pre-commit.ci

pyscriptjs/src/plugin.ts

madhur-tandon · 2023-04-13T18:45:34Z

pyscriptjs/src/plugin.ts

+ * if not, throw an error explaining the bad value.
+ * This is the most generic validation function; other validation functions for common situations follow
+ * @param options.config - The (extended) AppConfig object from py-config
+ * @param {string} options.name - The name of the key in py-config to be checked


Is @param {string} options.name the correct way of specifying params in the docs?

I ask since -- in other places, for example: @param filePath is written i.e. without the type. And name of the variable is written first. The above example I gave is present in fetchJSPlugin function in main.ts

Not sure what the best standards are, but wanted to highlight regardless...

Thanks for calling it out - I think the syntax here is correct? I'm not a JSDoc expert - I'm following the @param documentation, specifically the Documenting a parameter's properties section about adding types and a parameter's properties.

madhur-tandon · 2023-04-13T18:51:42Z

pyscriptjs/src/plugin.ts

 }
+
+// Members of py-config in plug that we want to validate must be one of these types
+type BaseConfigObject = string | boolean | number;


Do you want to allow undefined here perhaps?

I don't think we want to - an undefined value coming from py-config is how we signal that the plugin should supply a default value:

https://github.com/JeffersGlass/pyscript/blob/3fa082df26f1a1d702779e1ac93a875426e71313/pyscriptjs/src/plugin.ts#L299-L308

So if a plugin wants to identify whether the value was left undefined in the config, they should set a sentinel value as the default and look for that at runtime.

I only ask since you typecast it explicitly here: https://github.com/JeffersGlass/pyscript/blob/3fa082df26f1a1d702779e1ac93a875426e71313/pyscriptjs/src/plugin.ts#L298

and check for undefined in the very next line.

Ahhh I see now - sorry was writing the merge as you posted this.

madhur-tandon

Tests are Great! Thanks @JeffersGlass

JeffersGlass added the status: WIP PR that is not yet ready for review label Mar 29, 2023

JeffersGlass marked this pull request as draft March 29, 2023 02:20

JeffersGlass changed the title ~~Improve validate() function for plugin options~~ WIP: Improve validate() function for plugin options Mar 29, 2023

JeffersGlass added status: ready PR that is ready for review and removed status: WIP PR that is not yet ready for review labels Mar 29, 2023

JeffersGlass changed the title ~~WIP: Improve validate() function for plugin options~~ Improve validate() function for plugin options Mar 29, 2023

JeffersGlass marked this pull request as ready for review March 29, 2023 13:28

JeffersGlass requested a review from antocuni March 29, 2023 13:28

antocuni reviewed Mar 29, 2023

View reviewed changes

pyscriptjs/src/plugins/pyterminal.ts Outdated Show resolved Hide resolved

JeffersGlass added status: WIP PR that is not yet ready for review and removed status: ready PR that is ready for review labels Mar 29, 2023

JeffersGlass marked this pull request as draft March 29, 2023 20:03

JeffersGlass mentioned this pull request Mar 30, 2023

Xterm.js Option #1317

Merged

14 tasks

JeffersGlass marked this pull request as ready for review April 3, 2023 15:57

JeffersGlass requested a review from antocuni April 3, 2023 15:57

JeffersGlass force-pushed the validation-typing branch from 29f7ac2 to bed4d8f Compare April 6, 2023 18:28

JeffersGlass and others added 11 commits April 6, 2023 13:29

Refactor validation with improved typing

53a81d8

[pre-commit.ci] auto fixes from pre-commit.com hooks

adfdbc8

for more information, see https://pre-commit.ci

Merge from pre-commit

0b1f52c

[pre-commit.ci] auto fixes from pre-commit.com hooks

c97dd95

for more information, see https://pre-commit.ci

Fix spelling

0aec7b4

Use JSON.stringify for clarity

19a5d68

Write tests

bc900a9

Begin option for validation function

4b60b0b

Refactor validation functions

28778f4

Docstrings for functions

436ff32

Remove dead comments

f9f30ee

JeffersGlass added 2 commits April 6, 2023 13:29

Remove more dead comments

1d1b982

Cleanup comments and jsdoc

3fa082d

JeffersGlass force-pushed the validation-typing branch from bed4d8f to 3fa082d Compare April 6, 2023 18:29

JeffersGlass added status: ready PR that is ready for review and removed status: WIP PR that is not yet ready for review labels Apr 6, 2023

madhur-tandon reviewed Apr 13, 2023

View reviewed changes

pyscriptjs/src/plugin.ts Show resolved Hide resolved

madhur-tandon reviewed Apr 13, 2023

View reviewed changes

madhur-tandon approved these changes Apr 13, 2023

View reviewed changes

Improve docstrings

0cd706d

JeffersGlass merged commit dfa116e into pyscript:main Apr 13, 2023

JeffersGlass mentioned this pull request Apr 13, 2023

Improve py-config parameter Validation #1387

Closed

Improve validate() function for plugin options #1323

Improve validate() function for plugin options #1323

Uh oh!

Conversation

JeffersGlass commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antocuni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JeffersGlass commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeffersGlass commented Mar 29, 2023

Uh oh!

JeffersGlass commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

madhur-tandon Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

JeffersGlass Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

madhur-tandon Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

JeffersGlass Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

madhur-tandon Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

JeffersGlass Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

madhur-tandon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Improve `validate()` function for plugin options #1323

Improve `validate()` function for plugin options #1323

JeffersGlass commented Mar 29, 2023 •

edited

Loading

JeffersGlass commented Mar 29, 2023 •

edited

Loading

JeffersGlass commented Apr 3, 2023 •

edited

Loading