Skip to content

improved parameter validation - #2302

Merged
lmccart merged 20 commits into
processing:masterfrom
Spongman:validation2
Dec 27, 2017
Merged

improved parameter validation#2302
lmccart merged 20 commits into
processing:masterfrom
Spongman:validation2

Conversation

@Spongman

@Spongman Spongman commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

this PR improves the parameter validation a little:

  • it now handles array types (eg. Number[]) and checks the types of the array's elements.
  • it checks object types (eg. p5.Color)
  • it checks Constant values (eg. HSB|RGB, etc...)
  • it checks Integer values are integral. (and updated doc comments where appropriate)
  • it pre-calculates more of the work needed to do the validation, and caches the lookupParamDoc results. performance is significantly improved in debug.
  • validation has been added to more p5 methods.
  • the validation code & data.json is excluded from IS_MINIFIED builds, reducing the un-deflated size from 1,100KB to 389KB.

fixes #1329, #785

Spongman added 4 commits October 24, 2017 17:21
# Conflicts:
#	src/core/error_helpers.js
#	src/webgl/p5.Matrix.js
#	src/webgl/p5.RendererGL.js
remove some of the debug code from minified build

# Conflicts:
#	src/core/error_helpers.js
@Spongman

Copy link
Copy Markdown
Contributor Author

it looks like everything changed in error_helpers.js but that because I had to split apart the parameter validation (which is conditional on IS_MINIFIED) and the top-level warning stuff (which is not). also, jslint is fussy about the ordering of functions inside {} blocks (but not elsewhere, for some reason).

@meiamsome

Copy link
Copy Markdown
Member

This looks like good improvements. It may be worth cherry picking 68de25a and 464c2f5 from #2177 into this PR & then that PR can be rebased onto this.

@Spongman

Spongman commented Oct 25, 2017

Copy link
Copy Markdown
Contributor Author

ok, this does the right thing with the argument count error. i fixed up some docs to accomodate.

i also added the location of the error in the message, which you can click on to navigate in chrome's devtools .

i'm not going to do any rebasing, because, for me at least, it always ends in tears.

@meiamsome

Copy link
Copy Markdown
Member

Ah, that's good that 2bb7aa removed the need for that extra parameter to createCanvas! It's a shame that Travis has your account blacklisted and won't run the tests on this PR - perhaps you can give them a shout to un-blacklist you?

@Spongman

Copy link
Copy Markdown
Contributor Author

travis? blacklisted? i have no idea what that is.

@meiamsome

meiamsome commented Oct 25, 2017

Copy link
Copy Markdown
Member

It's the thing that's supposed to automatically run tests on PRs to check they run fine. (The tick/cross next to the PR in the PR list)

Your PRs are marked as "abuse detected" here: https://travis-ci.org/processing/p5.js/requests
I'm not sure why, though, travis-ci/travis-ci#6942 says you should just email them to get it cleared up!

Either way, I'm just confirming this passes tests and afaics is ready for merge!

@Spongman

Copy link
Copy Markdown
Contributor Author

i'm sorry if I messed up the merge, it seems like i have a love/hate relationship with git: it just loves to hate me.

@lmccart
lmccart merged commit fc3c57b into processing:master Dec 27, 2017
lmccart pushed a commit that referenced this pull request Dec 27, 2017
@lmccart

lmccart commented Dec 27, 2017

Copy link
Copy Markdown
Member

this is great, sorry it took so long to merge

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.

Use IS_MINIFIED to disable friendly debugger in minified builds

3 participants