-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Version 1.4.1 adds reserved function “size” #5753
Comments
As far as I can see there is no |
Can you share a link to the sketch you are using to test this? Thanks. |
Hello, yes it’s an incomplete sketch, but it has a class that uses “size” as one of the attributes. But, even when I change those variable names inside the class, there is still a variable “size” in the main sketch and I still get the warning. I even went to some of my old sketches and updated the source from 1.4.0. to 1.4.1 and I get the same warning on those (because I frequently use “size” as a variable name. |
@almchung Can you perhaps have a look at this as I think it may be related to some FES bug? |
It looks like this line in the FES source is getting its list of reserved function names from the names of all the methods on all p5 classes:
I wonder if it's marking p5.js/src/data/p5.TypedDict.js Line 118 in 374acfb
|
so what is the solution to reserved function "size"? |
For now, you can ignore it, it's FES misreporting it as a reserved word when it isn't really. If someone wants to make a PR to fix this, I believe they would need to update this line to only include some p5 constructors instead of all of them:
I think maybe just |
But what to do if the code doesn't run cause of the issue? How can I ignore it? |
Does it go away if you use |
@mishaushev FES warnings does not cause the sketch to halt, it merely prints the message to console. If your code is halting there probably is another problem causing an actual error. |
These seem right to me. Looping in @limzykenneth @almchung to double check. Thanks! |
|
I would love to help fixing this one. I had a look at the existing code and understand why the current code is inaccurate in determining reserved function names. I tried another method such as the following
This gives us something very close to what we find in the reference documentation here: https://p5js.org/reference/ So before I get into implementation, I would like to understand, what functions do we want to warn about overriding exactly? I would imagine it should be any function which added by p5 on the global context |
Thanks for your interest @Ucodia! I think the issue is that on this line
p5Constructors ), we can fix this issue.
|
Got it, thanks I'll experiment with the suggestion and report the list of words that comes out of it. |
@davepagurek I think I can do it. I just want to ask that if we only want to include 4 classes ( namely p5, p5.Renderer, p5.RendererGL, and p5.Renderer2D ) can't we do it by being explicit about it and add them directly on see here for more details
|
@aditya-shrivastavv Agreed, it's not necessary to literally use |
Please assign this issue to me, I am working on it. |
Hi everyone. I revisited this issue while reviewing a PR and wanted to outline reserved function Friendly Errors issues and propose another approach to resolving this problem. To provide some context, there are currently 326 reserved functions that should NOT be overridden by a user. (v.1.6.0)
I feel that if we can manage to make this approach (in 3) work, then we may not have to worry about false positives/negatives, so I want to know what others think. I am hopeful that this approach would work, but I admit I may be missing something. |
Thanks @almchung! I think that approach sounds good and would avoid some of the current issues we have, so if @aditya-shrivastavv feels like taking it on, I'd support that, but otherwise I think it's fine to accept incremental progress on the current system in the mean time. I think the approach is sound as long as we aren't dynamically assigning values to
Are those false negatives caught by main.js L691-L695? If so, I think it's OK to proceed with the change. Otherwise, agreed, we probably want to try to catch these in some other way. |
One other concern would be a dependency on having a full build. We don't support anything else currently, but there are some open issues discussing paths to being able to only include used files when importing p5 (#5740), so we'd probably want to think about how this would affect a future where that's possible. It might mean that generating the list of reserved strings is a separate build command that builds a full p5 release, then extracts function names via a regex, then writes the result out to a file, which is then read as a regular array of strings during regular builds. |
@davepagurek: Sounds great! I also agree we should work on this issue incrementally.
Yes, main.js L691-L695 will catch most cases, just excluding ~10 false negatives listed in 1-ii. |
Topic
Between version 1.4.0 and 1.4.1 a reserved function “size” was added. I feel like this variable name is used way too commonly in sketches to have it as a reserved function.
The text was updated successfully, but these errors were encountered: