incorrect constructor documentation

[pixelzoom]

Related to #1 (code review).

Documentation of parameters does not match constructor signature:

class XrayControlPanel extends Panel {

    /**
     * @param {WavesModel} model
     * @param {AlignGroup} alignGroup
     * @param {Object} [options]
     */
    constructor( model ) {

[pixelzoom]

{WavesModel} is also not a define type.

Also in XrayParameterPanel.js:

    /**
     * @param {WavesModel} model
     * @param {Object} [options]
     */
    constructor( model, options ) {

[heldentodd]

I'm a little confused with the constructors, especially when I'm passing something like the whole model. I've changed these to:

  class XrayControlPanel extends Panel {

    /**
     * @param {XrayDiffractionModel} model
     */

    constructor( model ) {

and

  class XrayParameterPanel extends Panel {

    /**
     * @param {XrayDiffractionModel} model
     * @param {Object} [options]
     */
    constructor( model, options ) {

[pixelzoom]

That looks perfect.

What bits are you confused about?

[heldentodd]

I know it's in the comments, but the flexibility bothers me a bit. When it's a standard data type like number or Vector3 it seems simple. The example with XrayParameterPanel shows the two very different types. In one case we have a very specific data structure - XrayDiffractionModel - which we sort of give up on and just use the name of the defining .js file (this is technically also true for something like Vector3). On the other hand, with options, we just use the most basic JavaScript type: "Object". It would be nice to somehow indicate the expected format of options here ({name: "value", ...}).

[pixelzoom]

The type expressions {XrayDiffractionModel} and {Vector3} both tell us to look for a class of that type, and find further details there. Primitive types, like {number} are more straightforward.

As for options... PhET struggled with whether to include a type expression for options. But we decided that we use options in a consistent manner, and they are always defined (and documented) at the beginning of the constructor. So including a type expression for options would therefore be redundant information that would likely get out-of-sync.

So PhET requires that each field in options must have a default value (so it's explicitly defined) and documentation that typically includes a type expression. E.g.:

/**
  * ...
  * @param {Object} [options]
  */
constructor( ..., options ) {

  options = merge( {

    // {Vector2} default position in the 2D model coordinate frame
    position: Vector2D.Zero,

    // {string} 'vertical' or 'horizontal'
    orientation: 'vertical',

    // {number} spacing between components, based on orientation
    spacing: 5,

    ...
  }, options );
} 

PhET developers know that options is always a JavaScript object literal, and that they can look inside the constructor for documentation on options field. If we were going to generate JSdoc, this would be a problem, but PhET currently is not generating documentation, and feels like there wouldn't be much benefit to doing so.

Does that help?

[heldentodd]

This is all very good to know. I think I have a handle on it now. I also believe the @params in the code are good now.