phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
54 stars 12 forks source link

How to handle generated function definitions #581

Closed jonathanolson closed 7 years ago

jonathanolson commented 7 years ago

Currently, there are a number of methods that are auto-generated (instead of being inside the inherit block). These may mess with editor auto-completion/inspections, and are generally harder to read and document.

Question:

Should these be expanded to normal definitions in the inherit block?

Current generated Scenery methods:

Node:

Line:

Rectangle:

Text:

pixelzoom commented 7 years ago

Based on the fact that the number of auto-generated functions is relatively small, my vote would be to expand them to normal definitions in the inherit block (i.e., manually define them).

samreid commented 7 years ago

+1 for seeing what they look like manually defined, perhaps putting these all close together and minimizing boilerplate (perhaps use line comment instead of full JSDoc?) so it is easy to see if one is incorrect.

jonathanolson commented 7 years ago

and minimizing boilerplate (perhaps use line comment instead of full JSDoc?)

If we expand these out, I'd prefer to keep things consistent with the rest of the documentation (for example, custom documentation on fontFamily would be helpful). It would also potentially help out with documentation generation in the future (since a lot of the documentation is moving from the manually-created HTML to the code).

jonathanolson commented 7 years ago

Expanded the ones for Node in https://github.com/phetsims/scenery/commit/7dbf9d83fdd239b374ece81810241d082ee1aa31 to check for size changes

Scenery (and dependencies):

So expansion would increase by 3933 bytes (~437 gzipped), approximately 0.3% of Scenery's size, or ~0.08% of an example sim (probably, used acid-base-solutions as example), and adds about ~536 LOC.

I'm currently leaning towards expansion, given that the major drawback (filesize) is ~0.4kB after gzip.

pixelzoom commented 7 years ago

The expanded functions (and especially their documentation) look nice in Node. I think that ~0.4kB size increase is a negligible price to pay for this improvement.

samreid commented 7 years ago

Looks great, let's continue.

jonathanolson commented 7 years ago

Handled in branch, will merge to master after testing.