{"action":"created","issue":{"url":"https://api.github.com/repos/processing/p5.js/issues/1287","repository_url":"https://api.github.com/repos/processing/p5.js","labels_url":"https://api.github.com/repos/processing/p5.js/issues/1287/labels{/name}","comments_url":"https://api.github.com/repos/processing/p5.js/issues/1287/comments","events_url":"https://api.github.com/repos/processing/p5.js/issues/1287/events","html_url":"https://github.com/processing/p5.js/pull/1287","id":138676648,"number":1287,"title":"[WIP] Add experimental type-checker for reference tests","user":{"login":"toolness","id":124687,"avatar_url":"https://avatars.githubusercontent.com/u/124687?v=3","gravatar_id":"","url":"https://api.github.com/users/toolness","html_url":"https://github.com/toolness","followers_url":"https://api.github.com/users/toolness/followers","following_url":"https://api.github.com/users/toolness/following{/other_user}","gists_url":"https://api.github.com/users/toolness/gists{/gist_id}","starred_url":"https://api.github.com/users/toolness/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/toolness/subscriptions","organizations_url":"https://api.github.com/users/toolness/orgs","repos_url":"https://api.github.com/users/toolness/repos","events_url":"https://api.github.com/users/toolness/events{/privacy}","received_events_url":"https://api.github.com/users/toolness/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-03-05T12:44:05Z","updated_at":"2016-03-05T20:59:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/processing/p5.js/pulls/1287","html_url":"https://github.com/processing/p5.js/pull/1287","diff_url":"https://github.com/processing/p5.js/pull/1287.diff","patch_url":"https://github.com/processing/p5.js/pull/1287.patch"},"body":"**This is a [work-in-progress pull request](http://vrybas.github.io/blog/2014/04/11/wip-pull-requests/), please don't merge it yet.**\r\n\r\n@lmccart: let me know if it's annoying to have a WIP PR lying about here--it's fine if that's the case, I can just move it over to my fork. I thought it might be nice to have it in the p5.js PR list so others can participate.\r\n\r\nThis is an attempt to use p5's own yuidocs to actively type-check its own examples. This can be beneficial for a number of reasons:\r\n\r\n* It ensures that our documentation is accurate. For example, if `lerpColor()` can actually take a `p5.Color` as a first argument--*and* it's likely that users will want to do this--but it isn't documented, then we should document it. Or if the argument type `Number` is misspelled as `Nimber`, we should throw an error at build or test time.\r\n* It means that we can *potentially* use our yuidoc metadata to build a more robust friendly error system, as per #759.\r\n\r\nLimitations of this PR:\r\n\r\n* Eventually, we should support some way of providing custom validation for methods that have complicated or problematic signatures, like [`save()`](http://p5js.org/reference/#/p5/save). There's a number of ways we can do this but for now we'll use one of two strategies:\r\n  * Store a blacklist that excludes such methods, or even entire files, from validation entirely. This is defined as `FILES_TO_IGNORE_FOR_NOW` and `METHODS_TO_IGNORE_FOR_NOW` in `test/js/typeChecker.js`.\r\n  * If the custom validation isn't very complex, we can write our own custom validator for now by adding an entry to the `customValidators` mapping in `test/js/typeChecker.js`.\r\n* Ideally, we'd also type-check against the standard test suite, but this introduces complications to the build system, since it means that we'd have to generate yuidocs metadata before running tests, so we'll deal with that later if this PR is successful.\r\n* This doesn't actually implement #759, it just lays some groundwork for doing so, while also providing the benefit of ensuring the continual correctness of our documentation. After all, before actually implementing #759, we need to make sure that the advice we're giving users in our friendly error system is actually accurate! :grin: \r\n* This PR doesn't currently check that the *return value* of methods is actually what they're documented to be; this can be added in a future PR too.\r\n\r\nOther concerns:\r\n\r\n* It's possible that some methods may *secretly* accept parameters that we want to *keep* undocumented. For example, `p5.color()` can actually take a `p5.Color` instance as its single argument, and internal p5 methods do this all the time. I'm not entirely sure what the point of this is, and it may be too confusing for beginners to document this in our yuidocs, so I wrote a custom validator for this method that makes it not complain if the first argument is a `p5.Color`.\r\n* In order for this to be really useful as p5 evolves, we need to actually document the functionality well and provide useful error feedback. Otherwise contributors will just get confused when they change the code and this particular test suite barfs at them with no guidance on how to fix things.\r\n* If folks generally think this PR is a good idea, it'd be nice to figure out a way to get it into the codebase sooner rather than later. Making *all* examples pass type-checking appears to be quite a task, which would result in a potentially massive PR; one alternative may be to *whitelist* the examples we type-check at first, so that we start out by type-checking just a few of them, and slowly expand the coverage through subsequent PRs, until finally we switch from whitelisting to blacklisting.\r\n\r\nFinally, note that this PR is an experiment! It's okay if it ends up being rejected. Hopefully we can learn from its mistakes, at least.\r\n"},"comment":{"url":"https://api.github.com/repos/processing/p5.js/issues/comments/192735393","html_url":"https://github.com/processing/p5.js/pull/1287#issuecomment-192735393","issue_url":"https://api.github.com/repos/processing/p5.js/issues/1287","id":192735393,"user":{"login":"lmccart","id":191056,"avatar_url":"https://avatars.githubusercontent.com/u/191056?v=3","gravatar_id":"","url":"https://api.github.com/users/lmccart","html_url":"https://github.com/lmccart","followers_url":"https://api.github.com/users/lmccart/followers","following_url":"https://api.github.com/users/lmccart/following{/other_user}","gists_url":"https://api.github.com/users/lmccart/gists{/gist_id}","starred_url":"https://api.github.com/users/lmccart/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lmccart/subscriptions","organizations_url":"https://api.github.com/users/lmccart/orgs","repos_url":"https://api.github.com/users/lmccart/repos","events_url":"https://api.github.com/users/lmccart/events{/privacy}","received_events_url":"https://api.github.com/users/lmccart/received_events","type":"User","site_admin":false},"created_at":"2016-03-05T20:59:44Z","updated_at":"2016-03-05T20:59:44Z","body":"This is very exciting. I like that it forces us to keep the documentation updated and correct.\r\n\r\nRegarding your questions, I guess the whitelist with eventual switch to blacklist makes sense and sounds most doable. It seems to make more sense to have you guys focus your time with this effort on the structural stuff rather than reviewing the documentation for every example, so let's not let that hold you up.\r\n\r\nI'm not sure what to do about the \"secret\" arg acceptance... maybe we need to add an option into yuidocs for params/signatures that aren't documented but are accepted?\r\n\r\nA tangential question that has come up for me is the way we display params in the reference. It works fine when there is an optional argument, we just use [ ] to make this clear. It gets more confusing when there are actually different sets up arguments that could fit. Then we end up with confusion params documentation like this:\r\n```javascript\r\n * @method color\r\n * @param  {Number|String} v1   gray value or red or hue value relative to\r\n *                                                  the current color range, or a color string\r\n * @param  {Number}        [v2]    gray value or green or saturation value\r\n *                                                  relative to the current color range (or\r\n *                                                  alpha value if first param is gray value)\r\n * @param  {Number}        [v3]    gray value or blue or brightness value\r\n *                                                  relative to the current color range\r\n * @param  {Number}        [alpha] alpha value relative to current color range\r\n```\r\n\r\nor \r\n\r\n```javascript\r\n* @param  {Array|Number|p5.Color} c1  interpolate from this color\r\n* @param  {Array/Number} c2  interpolate to this color\r\n* @param  {Array|Number|p5.Color} c2  interpolate to this color\r\n```\r\n\r\nIt's not totally clear which ones get used in combination with each other. I think the examples help a lot, but I really like the way that the processing ref just shows each different option under syntax:\r\nhttps://processing.org/reference/color_.html\r\n\r\nI wonder if it's possible to change the rendering of the ref pages to include multiple options like this, and also that that might mean for the way we are documenting inline."}}