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

Tighten up accessibility annotations (public, protected, private) #223

Closed
pixelzoom opened this issue Mar 3, 2024 · 2 comments
Closed
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 3, 2024

For code review #32 ...

There are quite a few things with accessibility public that don't need to be part of the public API, and should be protected or private.

For example in Field.ts:

-  public readonly meanSpeedProperty: DynamicProperty<number, number, Launcher>;
+  private readonly meanSpeedProperty: DynamicProperty<number, number, Launcher>;

-  public readonly standardDeviationSpeedProperty: DynamicProperty<number, number, Launcher>;
+  private readonly standardDeviationSpeedProperty: DynamicProperty<number, number, Launcher>;

-  public readonly abstract selectedSampleNumberProperty: NumberProperty;
+  protected readonly abstract selectedSampleNumberProperty: NumberProperty;

-  public readonly abstract identifier: string;
+  protected readonly abstract identifier: string;

Then there are constructors for base classes that are not intended to be directly instantiated, and should therefore be protected: VSMModel, SMModel, HeatMapToolNode, and probably others.

Was your default to start with public and then narrow later? If so, you might want to reconsider that approach -- start with private and widen as needed, so that your IDE/TypeScript helps get the annotation correct.

Also recommended to review this sim again to tighten up the accessibility annotations.

@samreid
Copy link
Member

samreid commented Mar 4, 2024

@matthew-blackman and I wrote a draft script to help us with access modifiers:

// Iterate through all the public and protected access modifiers within the js/ directory for the specified repo.
// For each one:
// 1. Change it to private. Then run the type checker to see if any errors are thrown.
//    - If it passes the type checker, keep it.
//    - If it fails, escalate from private => protected => public, testing at each level.

import { Project } from 'ts-morph';
import { execSync } from 'child_process';

// Initialize a new ts-morph Project
const project = new Project( {
  // Assuming tsconfig.json is in the root, adjust if necessary
  tsConfigFilePath: 'tsconfig.json'
} );

// Function to tighten accessibility annotations
async function tightenAccessModifiers() {
  const sourceFiles = project.getSourceFiles( 'js/**/*.ts' ); // Adjust the glob pattern as necessary

  for ( const sourceFile of sourceFiles ) {
    const classes = sourceFile.getClasses();

    for ( const classDeclaration of classes ) {


      // if ( classDeclaration.getName() !== 'PDLUtils' ) {
      //   continue;
      // }

      console.log( `# Processing class: ${classDeclaration.getName()}` );


      const members = [
        ...classDeclaration.getInstanceProperties(),
        ...classDeclaration.getInstanceMethods(),
        ...classDeclaration.getStaticProperties(),
        ...classDeclaration.getStaticMethods()
      ];

      for ( const member of members ) {


        // if ( member.getName() !== 'meanSpeedProperty' ) {
        //   continue;
        // }

        console.log( member.getScope() + ' ' + member.getName() );

        if ( member.getScope() === 'public' || member.getScope() === 'protected' ) { // Correct way to check for public modifier using ts-morph
          // console.log( `  Found public member: ${member.getName()}, attempting to set to private.` );

          // Try setting to private
          member.setScope( 'private' );
          await sourceFile.save();

          if ( !isBuildSuccessful() ) {
            // console.log( `    Setting ${member.getName()} to private failed, attempting to set to protected.` );

            // If not successful, try protected
            member.setScope( 'protected' );
            await sourceFile.save();

            if ( !isBuildSuccessful() ) {
              // console.log( `    Setting ${member.getName()} to protected failed, reverting to public.` );

              // If still not successful, revert to public
              member.setScope( 'public' );
              await sourceFile.save();
            }
            else {
              console.log( `    Successfully changed ${member.getName()} to protected.` );
            }
          }
          else {
            console.log( `    Successfully changed ${member.getName()} to private.` );
          }
        }
      }
    }

    // Optionally save the changes here if needed
    await sourceFile.save();
  }
}

function isBuildSuccessful() {
  // console.log( 'isBuildSuccessful???' );
  try {
    // Specify the path to the TypeScript compiler you want to use
    const tscPath = '/Users/samreid/phet/root/chipper/node_modules/typescript/bin/tsc';

    // Run the specified TypeScript compiler in the current directory
    execSync( `node ${tscPath}` );

    // If tsc exits without error, the build is successful
    // console.log( 'Build successful' );
    return true;
  }
  catch( error ) {
    // If tsc exits with an error (non-zero exit code), the build failed
    // console.error( 'TypeScript compilation failed' );
    return false;
  }
}

// Run the script
tightenAccessModifiers().then( () => console.log( 'Finished processing files.' ) );

@samreid
Copy link
Member

samreid commented Mar 5, 2024

@matthew-blackman and I ran the script and will continue in phetsims/chipper#1421. Closing.

@samreid samreid closed this as completed Mar 5, 2024
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