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

Discuss documentation for class properties #1201

Closed
chrisklus opened this issue Feb 28, 2022 · 3 comments
Closed

Discuss documentation for class properties #1201

chrisklus opened this issue Feb 28, 2022 · 3 comments
Assignees

Comments

@chrisklus
Copy link
Contributor

From the conventions doc:

Documentation for class properties should be placed with the declaration, not the instantiation.

How has this been working for out for everyone? I have found myself wanting to keep documentation at the place of instantiation. I think this is because the initial value is generally valuable to me for learning about the property. For simple types, it's usually pretty clear what the type is at instantiation, and for Properties, all of the type information is duplicated anyway, so it's more helpful for me to see the type info plus the initial value. In cases like DerivedProperties, it's much more helpful to see the comment with the derivation:

// how long the shape is visible when shown, in seconds. This is a derived Property instead of a constant because
// the time that the shape is shown is increased if the user gets the answer wrong multiple times.
this.timeToShowShapeProperty = new DerivedProperty( [ numberOfAnswerButtonPressesProperty ],
  numberOfAnswerButtonPresses => {
    if ( numberOfAnswerButtonPresses > NumberPlayConstants.NUMBER_OF_SUBITIZER_GUESSES_AT_NORMAL_TIME ) {
      return this.timeToShowShapeProperty.value + NumberPlayConstants.SHAPE_VISIBLE_TIME_INCREASE_AMOUNT;
    }
    return NumberPlayQueryParameters.subitizeTimeShown;
  } );

A downside of this is that WebStorm by default navigates back to the place of declaration for command + B.

Happy to switch to the convention, but wanted to see what others have been experiencing with this.

@zepumph
Copy link
Member

zepumph commented Feb 28, 2022

This was discussed during today's dev meeting about typescript conventions.

@samreid and @jonathanolson like splitting the documentation up, where the public interface and explanation is in the class type declaration, but any implementation details about the instantiation still deserve to be at that point in the constructor.

A downside of this is that WebStorm by default navigates back to the place of declaration for command + B.

The team resonates with this as a point towards having the documentation split.

We will make sure this is documented in the typescript conventions doc, and in the CRC. Thanks @chrisklus.

@zepumph
Copy link
Member

zepumph commented Feb 28, 2022

We will also add that we prefer having the API doc for options where the option is defined in the type, instead of where the default value is defined.

@chrisklus
Copy link
Contributor Author

Update above, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants