Correct msgWithReference logic in FES to fix issue 5576 #6221

Ayush23Dash · 2023-06-18T19:03:34Z

Resolves [#5576 ]

Changes:

Whenever a function having p5.[Class] in its name encounters an error, we need to have the a different FES error link as compared to when a different p5 function name encounters error.
This issue fixes #5576

Screenshots of the change:

PR Checklist

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

welcome · 2023-06-18T19:03:36Z

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

nbriz · 2023-06-19T16:35:08Z

@Ayush23Dash the change u made makes sense + looks good to me as far as i can tell.

that said, when testing this locally i noticed something potentially off. first i tested issue #5576 by reproducing it locally using the version of p5.js i had cloned at the start of GSoC, as expected i got the same error reported in the issue with the link to the wrong doc:

🌸 p5.js says: [sketch.js, line 6] An error with message "file.type is undefined" occurred inside the p5js library when File was called. If not stated otherwise, it might be an issue with the arguments passed to File. (http://p5js.org/reference/#/p5/File)

looking at ur code it seems like a great way to fix the issue, but just to do my due diligence i pulled ur changes to my local machine, built the library (npm run build) and re-ran the test sketch to see if the URL was pointing to the right doc, but to my surprise I got an entirely different error message:

🌸 p5.js says: 
[p5-test.js, line 259076] Cannot read property of undefined. Check the line number in error and make sure the variable which is being operated is not undefined.

 + More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_access_property#what_went_wrong p5-test.js:109297:18
┌[http://localhost:8000/p5.js/lib/p5-test.js:259076:54] 
	 Error at line 259076 in File()
└[http://localhost:8000/FES_TESTER/sketch.js:6:11] 
	 Called from line 6 in setup()

realizing that the changes i pulled included a lot more than the change u had made (because it had been a couple of weeks since i pulled from upstream) I then ran the same experiment with the latest main branch and got the same result.

@almchung any idea why this is the case? i'm assuming what we would want to see in the console is something more like this right?

🌸 p5.js says: [sketch.js, line 6] An error with message "file.type is undefined" occurred inside the p5js library when File was called. If not stated otherwise, it might be an issue with the arguments passed to File. (http://p5js.org/reference/#/p5.File)

Ayush23Dash · 2023-06-20T07:33:56Z

Thanks for the thorough update @nbriz. This seems like a weird issue, I will check in and then discuss it over our meet!

nbriz · 2023-06-20T16:04:36Z

hey @Qianqianye + @almchung i just finished meeting with @Ayush23Dash && as mentioned in my previous comment it seems like there have been changes elsewhere in the code base that prevent me/us from fully testing Ayush's change, but I'm fairly certain the change made would solve issue #5576 so my recommendation would be to merge this && close that issue.

is that something I should do myself, or is that something u'll do @Qianqianye (i'm still learning the workflow + protocols 😅)

Qianqianye · 2023-06-20T19:43:20Z

Thanks @nbriz! I have added you to the list of contributors who can merge PRs directly, so please feel free to go ahead and merge this one. You can read more about the workflow in the steward guide. I also pasted the PR merge workflow below:

Once the PR has been reviewed and no additional changes are required, a steward can mark the PR as "Approved" by choosing the "Approve" option in the previous step (instead of "Comment" or "Request changes") with or without additional comments. The steward can then either request additional review by another steward or maintainer if desired, or merge the PR if they have merge access or a maintainer will merge the approved PR.

Ayush23Dash · 2023-06-21T05:01:39Z

Hey @Qianqianye @nbriz
Just one final question regarding this : Should I squash and rebase the commit before Nick merges it?
Or we can merge it as it is!?

asukaminato0721 · 2023-06-21T11:56:55Z

src/core/friendly_errors/fes_core.js

-      msgWithReference = `${message} (http://p5js.org/reference/#/${referenceSection}/${funcName})`;
+
+      //Whenever func having p5.[Class] is encountered, we need to have the error link as mentioned below else different link
+      funcName.substring(0,2) === 'p5.' ?


funcName.startsWith('p5.') ?

oh wow! when was this method added to JS strings? don't think i'd seen it before. thnx @wuyudi

it's not completely necessary, but it would be easier for other contributors to read/understand, what do u think @Ayush23Dash

when was this method added to JS strings?

es6, https://stackoverflow.com/a/646643/13040423

We can use this @nbriz! Thanks @wuyudi, Ill just update the PR

nbriz · 2023-06-21T15:34:07Z

ah! thnx for pointing me to the steward guide @Qianqianye just read through it (makes perfect sense), only question i have is where do i call the "all-contributors"? is that a comment i should leave here in the PR (before or after it's closed? does it matter?) or somewhere else?

@Ayush23Dash as far as I can tell from looking at the rebase and resolve conflicts in the contributor guidelines, nothing more is needed git-wise (seeing as there are no conflicts && only the necessary files were changed + commits made) but considering that this is our first GSoC FES PR, i'll let @Qianqianye confirm this before i merge :)

nbriz · 2023-06-21T17:58:12Z

src/core/friendly_errors/fes_core.js

@@ -144,7 +144,7 @@ if (typeof IS_MINIFIED !== 'undefined') {
        methodParts.length === 1 ? func : methodParts.slice(2).join('/');

      //Whenever func having p5.[Class] is encountered, we need to have the error link as mentioned below else different link
-      funcName.substring(0,2) === 'p5.' ?
+      funcName.startsWith(0,2) === 'p5.' ?


did u test this? i think the first argument of startsWith needs to be a string

Apologies, fixed and tested it as well now!

almchung · 2023-06-24T00:52:11Z

Thank you so much for taking care of this issue!
@nbriz - To use the all-contributors bot, you can add a comment with the @all-contributors please add [...] command to this thread #2309 or to any other issue tickets or posts (including this PR).

nbriz · 2023-06-26T21:12:31Z

ok @Qianqianye @almchung @Ayush23Dash @wuyudi merging our first FES-GSoC-2023 PR! 🥳

@all-contributors please add @Ayush23Dash for 'code'
@all-contributors please add @nbriz for 'review'

allcontributors · 2023-06-26T21:12:34Z

@nbriz

This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 106406

Qianqianye · 2023-06-26T21:46:47Z

@all-contributors please add @Ayush23Dash for code

allcontributors · 2023-06-26T21:46:49Z

@Qianqianye

This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 106406

Qianqianye · 2023-06-26T21:55:32Z

@all-contributors please add @nbriz for review

allcontributors · 2023-06-26T21:55:41Z

@Qianqianye

I've put up a pull request to add @nbriz! 🎉

Qianqianye · 2023-06-26T21:56:01Z

@all-contributors please add @Ayush23Dash for code

allcontributors · 2023-06-26T21:56:10Z

@Qianqianye

I've put up a pull request to add @Ayush23Dash! 🎉

Qianqianye · 2023-06-26T21:57:47Z

Thank you all for working on this. There was a small error in the allcontributors doc earlier. I just added both @Ayush23Dash and @nbriz! 🎉

Correct msgWithReference logic in FES to fix issue 5576

98168c0

Fix indentation errors

8e472d0

Qianqianye requested review from nbriz and almchung June 19, 2023 16:24

asukaminato0721 reviewed Jun 21, 2023

View reviewed changes

Update .subtring to .startsWith

b531f0f

nbriz reviewed Jun 21, 2023

View reviewed changes

Correct .startsWith syntax

f70025d

processing deleted a comment from allcontributors bot Jun 24, 2023

nbriz merged commit 99526cd into processing:main Jun 26, 2023

allcontributors bot mentioned this pull request Jun 26, 2023

docs: add nbriz as a contributor for review #6241

Merged

allcontributors bot mentioned this pull request Jun 26, 2023

docs: add Ayush23Dash as a contributor for code #6242

Merged

Qianqianye mentioned this pull request Jun 26, 2023

solved the issue related to wrong link generation of p5.[Class] reference link. #6094

Closed

almchung mentioned this pull request Jul 4, 2023

1.7.0 Release #6215

Closed

4 tasks

Qianqianye added this to the 1.7.0 milestone Jul 4, 2023

Uh oh!

Correct msgWithReference logic in FES to fix issue 5576 #6221

Correct msgWithReference logic in FES to fix issue 5576 #6221

Uh oh!

Conversation

Ayush23Dash commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

welcome bot commented Jun 18, 2023

Uh oh!

nbriz commented Jun 19, 2023

Uh oh!

Ayush23Dash commented Jun 20, 2023

Uh oh!

nbriz commented Jun 20, 2023

Uh oh!

Qianqianye commented Jun 20, 2023

Uh oh!

Ayush23Dash commented Jun 21, 2023

Uh oh!

asukaminato0721 Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

nbriz Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

asukaminato0721 Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

Ayush23Dash Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

nbriz commented Jun 21, 2023

Uh oh!

nbriz Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush23Dash Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

almchung commented Jun 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbriz commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allcontributors bot commented Jun 26, 2023

Uh oh!

Qianqianye commented Jun 26, 2023

Uh oh!

allcontributors bot commented Jun 26, 2023

Uh oh!

Qianqianye commented Jun 26, 2023

Uh oh!

allcontributors bot commented Jun 26, 2023

Uh oh!

Qianqianye commented Jun 26, 2023

Uh oh!

allcontributors bot commented Jun 26, 2023

Uh oh!

Qianqianye commented Jun 26, 2023

Uh oh!

Uh oh!

Ayush23Dash commented Jun 18, 2023 •

edited

Loading

nbriz Jun 21, 2023 •

edited

Loading

almchung commented Jun 24, 2023 •

edited

Loading

nbriz commented Jun 26, 2023 •

edited

Loading