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

Typescript convention: omit "public" modifier, and generally cluster attributes by public, then private #1065

Closed
samreid opened this issue Jul 31, 2021 · 4 comments

Comments

@samreid
Copy link
Member

samreid commented Jul 31, 2021

The default visibility for typescript fields is public, and when asking WebStorm to convert a private to public member, it does so by removing the "private" modifier, not by switching to public. Still, things look a little less columnar if some things are marked with visibility and some are not. Here is an example from GravityAndOrbitsModel:

// no public
class GravityAndOrbitsModel {
  private showGravityForceProperty: BooleanProperty;
  private showVelocityProperty: BooleanProperty;
  private showPathProperty: BooleanProperty;
  private showGridProperty: BooleanProperty;
  private showMassProperty: BooleanProperty;
  private showMeasuringTapeProperty: BooleanProperty;
  isPlayingProperty: BooleanProperty;
  timeSpeedProperty: EnumerationProperty;
  gravityEnabledProperty: BooleanProperty;
  steppingProperty: BooleanProperty;
  rewindingProperty: BooleanProperty;
  private showMassCheckbox: boolean;
  private showMeasuringTape: boolean;
  private sceneList: SceneFactory;
  private sceneProperty: Property;
class GravityAndOrbitsModel {
  private showGravityForceProperty: BooleanProperty;
  private showVelocityProperty: BooleanProperty;
  private showPathProperty: BooleanProperty;
  private showGridProperty: BooleanProperty;
  private showMassProperty: BooleanProperty;
  private showMeasuringTapeProperty: BooleanProperty;
  public isPlayingProperty: BooleanProperty;
  public timeSpeedProperty: EnumerationProperty;
  public gravityEnabledProperty: BooleanProperty;
  public steppingProperty: BooleanProperty;
  public rewindingProperty: BooleanProperty;
  private showMassCheckbox: boolean;
  private showMeasuringTape: boolean;
  private sceneList: SceneFactory;
  private sceneProperty: Property;
  public static G: number;

I'm not sure which I like more, so I'd like to discuss it. I don't think we should say "developer preference" though--seems good to have a convention that we enforce through lint or code review. Labeling for #1049

@samreid
Copy link
Member Author

samreid commented Aug 28, 2021

Organizing by private vs public also seems best, then the public modifier is less important. This also has the advantage of making the public-facing API clear:

class GravityAndOrbitsModel {
  isPlayingProperty: BooleanProperty;
  timeSpeedProperty: EnumerationProperty;
  gravityEnabledProperty: BooleanProperty;
  steppingProperty: BooleanProperty;
  rewindingProperty: BooleanProperty;
  
  private showGravityForceProperty: BooleanProperty;
  private showVelocityProperty: BooleanProperty;
  private showPathProperty: BooleanProperty;
  private showGridProperty: BooleanProperty;
  private showMassProperty: BooleanProperty;
  private showMeasuringTapeProperty: BooleanProperty;
  private showMassCheckbox: boolean;
  private showMeasuringTape: boolean;
  private sceneList: SceneFactory;
  private sceneProperty: Property;
}

This issue seems at a good point to bring to dev meeting once other devs have some TS experience.

@samreid samreid changed the title Typescript convention: should we specify "public" for attributes? Typescript convention: omit "public" modifier, and generally cluster attributes by public, then private Sep 5, 2021
@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 11, 2021

At 11/4/21 dev meeting, we decided that using the public modifier was up to developer discretion.

And I may be hallucinating, but I've seen WebStorm reorganize my declarations so that they are in the order public, protected, private. I'm not sure what I did to make this happen.

EDIT: If you choose "Reformat Code" and check "Rearrange entries", then WebStorm will order your declarations by visibility. Unfortunately it also removes blank likes where PhET convention requires them (maybe a problem in phet-code-style.xml?)

@samreid
Copy link
Member Author

samreid commented Nov 11, 2021

I've documented the "developer preference to use public modifier` convention in https://github.com/phetsims/phet-info/blob/master/doc/typescript-conventions.md#visibility-annotations.

Now that I am getting better at reading these class attributes in TypeScript, I feel less strongly that we should require a convention that clusters by visibility. Can we also leave this as developer discretion?

@samreid
Copy link
Member Author

samreid commented Jul 15, 2022

We enabled a rule that requires visibility annotations. It is developer preference how (which order) to organize the properties. Closing.

@samreid samreid closed this as completed Jul 15, 2022
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

3 participants