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

[WIP] Add experimental type-checker for reference tests #1287

Closed
wants to merge 5 commits into from

Conversation

toolness
Copy link
Member

@toolness toolness commented Mar 5, 2016

This is a work-in-progress pull request, please don't merge it yet.

@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.

This 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:

  • 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.
  • It means that we can potentially use our yuidoc metadata to build a more robust friendly error system, as per Generate Friendly Error Messaging from existing documentation #759.

Limitations of this PR:

  • Eventually, we should support some way of providing custom validation for methods that have complicated or problematic signatures, like save(). There's a number of ways we can do this but for now we'll use one of two strategies:
    • 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.
    • 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.
  • 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.
  • This doesn't actually implement Generate Friendly Error Messaging from existing documentation #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 Generate Friendly Error Messaging from existing documentation #759, we need to make sure that the advice we're giving users in our friendly error system is actually accurate! 😁
  • 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.

Other concerns:

  • 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.
  • 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.
  • 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.

Finally, 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.

@lmccart
Copy link
Member

lmccart commented Mar 5, 2016

This is very exciting. I like that it forces us to keep the documentation updated and correct.

Regarding 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.

I'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?

A 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:

 * @method color
 * @param  {Number|String} v1   gray value or red or hue value relative to
 *                                                  the current color range, or a color string
 * @param  {Number}        [v2]    gray value or green or saturation value
 *                                                  relative to the current color range (or
 *                                                  alpha value if first param is gray value)
 * @param  {Number}        [v3]    gray value or blue or brightness value
 *                                                  relative to the current color range
 * @param  {Number}        [alpha] alpha value relative to current color range

or

* @param  {Array|Number|p5.Color} c1  interpolate from this color
* @param  {Array/Number} c2  interpolate to this color
* @param  {Array|Number|p5.Color} c2  interpolate to this color

It'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:
https://processing.org/reference/color_.html

I 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.

@toolness
Copy link
Member Author

toolness commented Mar 6, 2016

Awesome!

I totally agree regarding your tangential question, and I think it's important to figure it out sooner rather than later, as it will not only make the docs easier to understand, but the friendly error feedback too! I'll do some digging into yuidocs to see if there's something we can do.

Quick update: I found yui/yuidoc#190, which suggests that method overloading is sort-of supported. One possibility may be for me to contribute to yuidoc upstream so it supports this use case better.

Quick update update: Working on this now in #1295!

@toolness
Copy link
Member Author

Hey!! I just had an idea for a sort-of alternative approach to solving the same kind of problem as this PR, and I wrote a blog post about it here:

http://processing.toolness.org/general/2016/03/16/typescript-to-the-rescue.html

Technically the two approaches are independent of one another, and each is useful in its own way, I think, so we could potentially use both.

@lmccart
Copy link
Member

lmccart commented Mar 27, 2016

The typescript approach looks great, too! I also like that the typescript files generated could be used by users independently, if they had some other way they wanted to incorporate them. I agree about keeping things as DRY as possible -- because we have a lot of different people working on this project at different times and with different levels of experience and familiarity, anything that alerts us to errors or inconsistencies will further increase the sustainability of the project and the workflow we have.

@lmccart
Copy link
Member

lmccart commented Oct 12, 2016

hi @toolness I'm doing some cleanup here and wanted to check in on this pull request. this one says don't merge yet, are there further plans for this or should it be merged or closed? thanks!

@toolness
Copy link
Member Author

Oh shoot, sorry I have been totally delinquent on this! 😞 I think ultimately that the typescript approach might be better, because typescript is a really well-maintained project--and like you said, the TypeScript annotations can be useful on their own--which is unlike this blob of code in this PR, which I think could easily break as p5 evolves and be a pain to update. I think I'm going to close this PR for now, but we can always resurrect it if it turns out to be a better option!

@toolness toolness closed this Jan 17, 2017
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.

None yet

2 participants