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

Start developing a new param validator using Zod #7186

Open
wants to merge 3 commits into
base: dev-2.0
Choose a base branch
from

Conversation

sproutleaf
Copy link

Addresses #7178

Changes:

Add validate_params to p5.js. Currently, the file doesn't interact with p5 and does not have full functionality and documentation yet. It serves as a starting point for further development.

Screenshots of the change:

PR Checklist

};

function generateZodSchemasForFunc(func) {
const ichDot = func.lastIndexOf('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to readers to have a comment above this with some examples of the format of func that we expect. Based on the code below it looks like global functions might just be e.g. sin and class methods might be like p5.Vector.add?

Copy link
Author

Choose a reason for hiding this comment

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

I got this part from the original validate_params file and I believe that's the case!

const optional = param.endsWith('?');
param = param.replace(/\?$/, '');

if (param.includes('|')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might have to do something a bit more complicated to handle cases like the lerpPalette one, which ends up looking like [p5.Color, Number][]. A quick fix could be to just add that to the schema map for now though.

I'm also noticing that we have some options objects (e.g. for p5.Geometry.computeNormals()) that currently end up just typed as Object, whereas in docs/data.json it has this extra properties info that we don't yet use:

{
  "title": "param",
  "name": "options",
  "lineNumber": 11,
  "description": {
    "type": "root",
    "children": [
      {
        "type": "paragraph",
        "children": [
          {
            "type": "text",
            "value": "An optional object with configuration."
          }
        ]
      }
    ]
  },
  "type": {
    "type": "OptionalType",
    "expression": {
      "type": "NameExpression",
      "name": "Object"
    }
  },
  "properties": [
    {
      "title": "param",
      "name": "options.roundToPrecision",
      "lineNumber": 423,
      "default": "3"
    }
  ],
  "default": "{}"
}

That could be something we try to encode in the type eventually, e.g. instead of Object, it could be { roundToPrecision: number? }. We can decide that's out of scope for now because that would probably necessitate a recursive parser to extract the type info (or just more structured info in parameterData.json rather than just a type string.)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for bringing these edge cases up Dave! I'm adding both of these scenarios in my notes and will circle back to them later!


let funcSchemas = schemaRegistry.get(func);
if (!funcSchemas) {
funcSchemas = generateZodSchemasForFunc(func);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Comments look great!

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.

None yet

2 participants