Detecting redeclaration of p5.js reserved constants and functions #5351
Conversation
|
This is a draft PR |
|
|
||
| const IS_LOCAL = document.location.origin.includes('file://'); | ||
|
|
||
| if (typeof IS_MINIFIED !== 'undefined' || IS_LOCAL) { |
There was a problem hiding this comment.
I suggest making a variable with a descriptive name for this check (a.k.a. "what is this condition checking for?")
There was a problem hiding this comment.
do this for all variables across the file: for example instead of ele say element or whatever ele stands for. instead of arr say elementArray
There was a problem hiding this comment.
ok I will update the code
There was a problem hiding this comment.
Great progress! My main comment is please use descriptive variable names even if they are verbose. We do this to ensure human readability of the code particularly for novice contributors. We cannot have arrays called arr or strings called str. Also for variables like elements give a descriptive name.
| const checkForConstsAndFuncs = arr => { | ||
| let foundMatch = false; | ||
|
|
||
| for (let i = 0; i < arr.length; i++) { |
There was a problem hiding this comment.
instead of arr use a descriptive name for the array variablesArray. We do this in p5.js to make the code more accessible to new contributors and to other contributors that might be trying to understand what you are doing.
There was a problem hiding this comment.
ok I will make the changes soon
| for (let i = 0; i < arr.length; i++) { | ||
| //ignoreFunction contains the list of functions to be ignored | ||
| if (!ignoreFunction.includes(arr[i])) { | ||
| const keyArr = Object.keys(p5Constructors); |
| //the setup and draw function. If a match is found then | ||
| //to prevent further potential reporting we will exit immidiately | ||
| let moveAhead = globalConstFuncCheck(); | ||
| if (!moveAhead) { |
There was a problem hiding this comment.
Here you can use if (moveAhead) return to avoid extra indentation
There was a problem hiding this comment.
I will make the changes
There was a problem hiding this comment.
Great progress! Good job documenting the sketch_reader.js. Let's wait for @ghalestrilo feedback and then we can see if we're ready to merge :)
Aloneduckling
changed the title
| @@ -211,6 +245,8 @@ line(0, 0, 100, 100, x3, Math.PI); | |||
| * All the colors are checked for being color blind friendly. | |||
| * More elaborate ascii is always welcome! | |||
| * Extend Global Error catching. This means catching errors that the browser is throwing to the console and matching them with friendly messages. `fesErrorMonitor()` does this for a select few kinds of errors but help in supporting more is welcome :) | |||
| * Improve `sketch_reader.js`'s code reading and variable/function name extracting functionality. | |||
There was a problem hiding this comment.
be more detailed about what you mean with this. Why do we need this?
| @@ -211,6 +245,8 @@ line(0, 0, 100, 100, x3, Math.PI); | |||
| * All the colors are checked for being color blind friendly. | |||
| * More elaborate ascii is always welcome! | |||
| * Extend Global Error catching. This means catching errors that the browser is throwing to the console and matching them with friendly messages. `fesErrorMonitor()` does this for a select few kinds of errors but help in supporting more is welcome :) | |||
| * Improve `sketch_reader.js`'s code reading and variable/function name extracting functionality. | |||
| * `sketch_reader.js` can be extended and new features can be added to it to better aid the user. | |||
There was a problem hiding this comment.
I made the changes. Please have a look
Resolves #5350
Changes:
sketch_reader.jscontains the code that will help the FES detect redeclaration of p5.js constants and functions.Screenshots of the change:
Detection of p5.js reserved function:
Detection of p5.js reserved constant
PR Checklist
npm run lintpasses