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

FES: spelling and caps detection #4643

Merged
merged 6 commits into from
Jun 23, 2020

Conversation

akshay-99
Copy link
Member

@akshay-99 akshay-99 commented Jun 18, 2020

Resolves #4637

Changes:

I have tried the simpler check using toLowerCase() as suggested by @limzykenneth .
Seems to work well to capture all kinds of mistakes in capitalization. For the sketch

function Setup() {
  background(100);
}

and for the sketch

let sketch = function(p) {
    p.Setup = () => {
        p.background(100);
    }
}
let myp5 = new p5(sketch);

We get

🌸 p5.js says: It seems that you may have accidently written Setup instead of setup.

Please correct it if it's not intentional. (http://p5js.org/reference/#/p5/setup)

And for

function setup() {
  createcanvas(400, 400);
  background(100);
}

We get

🌸 p5.js says: It seems that you may have accidently written createcanvas instead of createCanvas (at http://localhost:8000/lib/empty-example/sketch.js:2:5).

Please correct it to createCanvas if you wish to use the function from p5.js (http://p5js.org/reference/#/p5/createCanvas)

Performance-wise the only concern was checkForUserDefinedFunctions as it would have to run the check regardless of whether there is an actual error or not. But it seems to run in under a millisecond and it will run only once, after p5 is initialized. fesErrorMonitor shouldn't be a problem for performance as it only runs when we have an actual error.

This only captures capitalization mistakes.
I will try to capture the ones for spelling now and also add tests

Update:

Now works for spelling mistakes too. For

function setup() {
  Colour();
}
🌸 p5.js says: It seems that you may have accidently written Colour instead of color (at http://localhost:8000/lib/empty-example/sketch.js:11:3).

Please correct it to color if you wish to use the function from p5.js (http://p5js.org/reference/#/p5/color)

Also added a few tests

PR Checklist

@akshay-99 akshay-99 force-pushed the fesSpellingAndCapsDetection branch from 055699d to 84b1556 Compare June 22, 2020 00:07
@akshay-99
Copy link
Member Author

akshay-99 commented Jun 22, 2020

I guess this is ready for review.

So as I mentioned in #4637, there are two types of checks:

  • Type 1:
    Mistakes in referencing p5 functions such as createCanvas, random, color, etc. I have used case-insensitive Levenshtein distance to detect these. The threshold is set to 2 for now. i.e it can detect CREATECANVAS, CrateCanvas,craeteCanvas etc. This is more of a "reactive" check in the sense that it reacts to ReferenceError messages, whenever they occur. It does not activate on its own.
    Different browsers use different error strings. It is much easier and cleaner if we divide these into categories so that we can extract the required information about the various kinds of errors.
    I have created a file browser_errors.js, in which we can add the various error strings used by browsers. We can later match error messages against this to give a simplified explanation for all kinds of errors. For now, I have just added two strings under ReferenceError. The first works for Chrome, Firefox, Edge and Opera. The second works for Safari.
    I have also excluded browser_errors.js from the minified build.

  • Type 2:
    This is for user-defined functions such as setup, draw, preload, mouseClicked, etc. This is a more proactive check, in the sense, it activates without an error message. For this, I have set it up so that it can only detect capitalization mistakes: SETUP, Draw, preLoad, etc. Adding the check for spelling too here would just give a whole bunch of false positives. As the user is allowed to define their own functions which are not related to p5 ( perhaps draw1, myDraw, etc. We should not trigger a message in these cases. ) Also, other libraries may define functions which are close in name to setup, draw, etc and so showing a message here for misspelling would cause more confusion. So just a capitalization check is enough here.

Type 2 works for both global and instance mode.
Type 1 works only for global mode. This is because, in instance mode, we won't have a ReferenceError. We could have a TypeError ( for lets say p.createcanvas() ) or no error at all. ( for eg p.mousey )
I thought about setting up a Proxy object, which can detect when a get() resolves to undefined, and pass that proxy to the sketch instead of p5. But this might be a bit too much and may break a few things. Also, older browsers and IE don't support Proxy and according to this page in the Babel docs, it can't be polyfilled either. So I decided to leave and not do that.

@akshay-99 akshay-99 marked this pull request as ready for review June 22, 2020 00:31
Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Fantastic! Just a few requests inline to help with clarity.

src/core/friendly_errors/browser_errors.js Show resolved Hide resolved
src/core/friendly_errors/browser_errors.js Outdated Show resolved Hide resolved
src/core/friendly_errors/fes_core.js Outdated Show resolved Hide resolved
src/core/friendly_errors/fes_core.js Outdated Show resolved Hide resolved
src/core/friendly_errors/fes_core.js Outdated Show resolved Hide resolved
@akshay-99
Copy link
Member Author

Also I just noticed that the error handler is not triggered when p5.sound.min.js is in the html file. I am looking into that

@akshay-99
Copy link
Member Author

@stalgiag I have made the changes as you suggested. Please let me know your views on this comment. If need be, I will change the browser: all thing too.

I have also fixed the problem of fesErrorMonitor not running when p5.sound.min.js is imported. Although I don't think I completely understand the reason I am presuming it must be because I had only set it to run when this promise fails ( an error in the sketch would make this happen ).

But when p5.sound or some other library is present, an error in the sketch would result in this other promise failing.

So I changed it to instead listen for any PromiseRejectionEvents or any ErrorEvents here

Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Great! This is a very exciting set of changes.

@stalgiag stalgiag merged commit e73cea7 into processing:main Jun 23, 2020
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.

FES: detect capitalization and spelling mistakes
2 participants