Prevent FES from checking nested properties

[IIITM-Jay]

Addresses #7752

Approach:

1. Filtering out the nested properties of a parameter, which to be treated as a part of parameter and not separate positional parameters by using the .includes() string method. If the parameter name contains a (.) (like options.extrude), we assume it’s nested, so skip it.
2. Keep the top-level parameters by including only those where t.name does not include a dot (.). Eg.options will be included, but options.extrude will be ignored.

Additional Issues Fix/ Changes Made:

1. The entry.params is checked with fallback now to prevent run time errors to throw in case of missing (i.e., undefined)
2. The if-else block is reduced to avoid redundancy.

Observations/ Results

Consider the list of params as below:

/**
   * @method createModel
   * @param  {String} modelString
   * @param  {String} [fileType]
   * @param  {Object} [options]
   * @param  {function(p5.Geometry)} [options.successCallback]
   * @param  {function(Event)} [options.failureCallback]
   * @param  {boolean} [options.normalize]
   * @param  {boolean} [options.flipU]
   * @param  {boolean} [options.flipV]
   * @return {p5.Geometry} the <a href="#/p5.Geometry">p5.Geometry</a> object
   */

Before with the existing code,

"createModel": {
      "overloads": [
          "String",
          "String?",
          "Object?",
          "function(p5.Geometry)?",
          "function(Event)?",
          "boolean?",
          "boolean?",
          "boolean?"
      ]
    }

After modifications, it will get documented as:

"createModel": {
      "overloads": [
          "String",
          "String?",
          "Object?"
      ]
    }

Testing/ Validation

Validated the logic to see the changes in parameterData.json artifact so generated. It modified the parameters of the overload function as expected.

PS: I have committed the file to visualize and validate the changes so occurerd.

[IIITM-Jay]

Hi @davepagurek May I request to have a look at this PR.

Based on the discussion happened in #7752 and as per the comment, I have planned to do it in two stages. One of which is in this PR - preventing FES to mistakenly checking the nested parameters (other positional parameters). The second stage that I am thinking is to make the properties of the Optional params to be more informative like a syntax below:

[
  "String",
  "String?",
  {
    "type": "Object",
    "optional": true,
    "properties": {
      "successCallback": "function(p5.Geometry)?",
      "failureCallback": "function(Event)?",
      "normalize": "boolean?",
      "flipU": "boolean?",
      "flipV": "boolean?"
    }
  }
]

The additional key as "optional": true to indicate that it is an optional parameter.

CC: @ksen0 @limzykenneth

[davepagurek]

The code looks good so far @IIITM-Jay! Could you run npm run docs and commit the results? We'll expect this to change parameterData.json for the entries that have nested properties, and seeing those changes in the PR will additionally be good validation that the logic has changed the intended spots.

[davepagurek]

For the second stage, it might make sense to apply the same format to all parameters. Currently, it looks like there are two different ways a parameter might be optional, either ending with a ? as in String? or via the optional: true parameter for an object. How do you feel about a format like this?

[
  { "type": "String", "optional": true },
  { "type": "String", "optional": true },
  {
    "type": "Object",
    "optional": true,
    "properties": {
      "successCallback":  { "type": "Function", "optional": true, parameters: [{ type: "p5.Geometry" }] },
      "failureCallback": { "type": "Function", "optional": true, parameters: [{ type: "Event" }] },
      "normalize": { "type": "boolean", "optional": true },
      "flipU": { "type": "boolean", "optional": true },
      "flipV": { "type": "boolean", "optional": true }
    }
  }
]

Some upsides would be:

• You could delete a lot of FES code that is trying to recursively parse complex type strings like (a | b)[] by just directly having a nested object to read
• You would have less duplication between object properties and other properties
• You could handle doubly-nested objects, e.g. if one of the success or failure callbacks has a parameter with object properties in the future, we could handle that here

Some downsides:

• parameterData.json would be bigger, as this format is more verbose
• It's more work to change -- you'd have to translate other types like arrays and tuples into a version of this format too

[IIITM-Jay]

Yeah Sure!
Committed the parameteData.json file

[davepagurek]

Awesome, those parameterData.json changes look like what we'd expect, so I think this is good to go when the automated tests finish!

[IIITM-Jay]

For the second stage, it might make sense to apply the same format to all parameters.

yes, I totally agree with this. Just a small observation from my side, here the first two are "optional": false. 😄

from your comments:

[
{ "type": "String", "optional": true },
{ "type": "String", "optional": true },

[davepagurek]

The tests failed but I'm pretty sure it's just due to a flaky test (#7823), I'm rerunning them now

[IIITM-Jay]

Some downsides:
parameterData.json would be bigger, as this format is more verbose
It's more work to change -- you'd have to translate other types like arrays and tuples into a version of this format too

Yes, @davepagurek I have already seen this and then after your suggestions I honestly felt to open a second PR for second stage possible more detailed and changes.

[IIITM-Jay]

Thanks a lot @davepagurek! for your time once again for insights and review.
Now the tests have passed. Enjoying this honestly

[davepagurek]

@all-contributors please add @IIITM-Jay for code

[allcontributors]

@davepagurek

I've put up a pull request to add @IIITM-Jay! 🎉

[IIITM-Jay]

The tests failed but I'm pretty sure it's just due to a flaky test (#7823), I'm rerunning them now

No issues @davepagurek ! we will fix this too! 🤗 , but will dig into this one first.!