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: Add detection for trailing undefined arguments #4606

Merged
merged 2 commits into from
Jun 8, 2020

Conversation

akshay-99
Copy link
Member

Resolves #4571

Changes:

  1. Make the FES detect trailing undefined arguments
    Consider the following few examples
let r, g, b;
function setup() {
  let r = 50;
  color(r, g, b);
}

Here g and b are undefined. But earlier as color with only one argument was still a valid overload, the FES did not log a message. But as it is passed 3 arguments, it is likely that them being undefined is unintentional and so a message should be displayed.

Now it shows:
🌸 p5.js says: color() was expecting Number for parameter #1 (zero-based index), received an empty variable instead

🌸 p5.js says: color() was expecting Number for parameter #2 (zero-based index), received an empty variable instead

Similarly:

function setup() {
  let a = document.getElementById('nonexistant');
  select('hgg', a);
}

would not have logged an error before this, as the undefined argument is trailing and select(string) is a valid overload. A message here is important to let the user know that a is empty ( i.e notexistant was not found )

Now it shows:
🌸 p5.js says: select() was expecting String|p5.Element|HTMLElement for parameter #1 (zero-based index), received an empty variable instead.

Also for

let r;
circle(10, 10, r);

Earlier we used to get
🌸 p5.js says: circle() was expecting at least 3 arguments, but received only 2

Now it correctly shows:
🌸 p5.js says: circle() was expecting Number for parameter #2 (zero-based index), received an empty variable instead.

  1. Pass the error type to the ValidationError constructor so that it could be accessed in tests
  2. Add tests for the edge cases in FES misses error cases #2740
  3. Add tests for detection of trailing undefined arguments

PR Checklist

1. Add tests for the edge cases in processing#2740
2. Pass errorType to ValidationError constructor so that it could
   be accessed in the tests
3. Add tests for trailing undefined arguments
@codecov-commenter
Copy link

Codecov Report

Merging #4606 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4606   +/-   ##
=======================================
  Coverage   67.73%   67.74%           
=======================================
  Files          73       73           
  Lines       11416    11420    +4     
  Branches     2442     2443    +1     
=======================================
+ Hits         7733     7736    +3     
- Misses       3683     3684    +1     
Impacted Files Coverage Δ
src/core/friendly_errors/validate_params.js 94.58% <100.00%> (+0.36%) ⬆️
src/webgl/loading.js 62.16% <0.00%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10a91f4...163ab1d. Read the comment docs.

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.

Looks great @akshay-99. Thank you!

@stalgiag stalgiag merged commit 1f3a028 into processing:master Jun 8, 2020
@stalgiag
Copy link
Contributor

stalgiag commented Jun 8, 2020

Hm - tests passed on this PR before but after merge we are failing on the selectAll() documentation.

EDIT: Ah yes, now I am remembering that the examples for save, selectAll and loadModels are broken with these changes. @akshay-99 do you want to open a new PR that makes the changes suggested in #4571 to fix these problems. The approach suggested by you (change arg length in save() and modify selectAll() example so that d and f exist seems like the best way forward.

@akshay-99
Copy link
Member Author

akshay-99 commented Jun 8, 2020

@stalgiag It seems it's because the example was updated again in #4602, which was merged after this PR was created. There needs to be a condition to check if c exists before line 105 here and also to see if b exists before line 101

@akshay-99
Copy link
Member Author

Ah yes, now I am remembering that the examples for save, selectAll and loadModels are broken with these changes.

The ones for save and loadModel are no longer broken as there it was a validation message due to an internal call to a library function from another library function which was suppressed in #4590. The example for selectAll was also fixed in #4580 but it was overwritten in #4602. I am making a PR to fix it.

@akshay-99
Copy link
Member Author

Whoops looks like @lmccart already fixed it. Thanks! 😄

@lmccart
Copy link
Member

lmccart commented Jun 8, 2020

oh sorry I missed this thread!

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: validateParameters does not detect trailing undefined arguments
4 participants