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

Move linting to perennial #1489

Closed
zepumph opened this issue Oct 11, 2024 · 12 comments
Closed

Move linting to perennial #1489

zepumph opened this issue Oct 11, 2024 · 12 comments

Comments

@zepumph
Copy link
Member

zepumph commented Oct 11, 2024

linting is a tool that is needed for perennial and for chipper. Let's have it be in the repo that is dependencyless.

  • moving grunt tasks (and forwarding chipper grunt tasks to perennial-alias's grunt)
  • moving all of chipper/eslint/ to perennial. (And keep history?!?!?!) Side issue: Move chipper/eslint/ to perennial #1500
  • moving all package.json entries for linting.
@zepumph zepumph changed the title Move lint.js perennial Move linting to perennial Oct 11, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 11, 2024
@samreid
Copy link
Member

samreid commented Oct 15, 2024

Do you prefer chipper/eslint/**/*.{js,mjs} moved to perennial/eslint or perennial/js/eslint?

@samreid samreid assigned zepumph and unassigned samreid Oct 15, 2024
@zepumph
Copy link
Member Author

zepumph commented Oct 15, 2024

I care most about how it holds the cache. Could we put everything in perennial/js/eslint and then save the cache to chipper/dist?

@zepumph zepumph assigned samreid and unassigned zepumph Oct 15, 2024
@zepumph
Copy link
Member Author

zepumph commented Oct 15, 2024

  • Also noting that as part of this issue I think cd perennial-alias; grunt lint should lint perennial-alias.

I just got bit by this in #1492 and needed to instead run grunt lint --repo=perennial-alias.

@zepumph
Copy link
Member Author

zepumph commented Oct 16, 2024

  • Noting that we should discuss the naming of these tasks. We have opted for a different patter with grunt check and output-js. It may be nice to include grunt lint --all which is the same as grunt lint-everything now.

samreid added a commit that referenced this issue Oct 17, 2024
samreid added a commit that referenced this issue Oct 17, 2024
@samreid
Copy link
Member

samreid commented Oct 17, 2024

In-progress patch:

Subject: [PATCH] chmod +x sage, see https://github.com/phetsims/chipper/issues/1481
---
Index: perennial/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial/tsconfig.json b/perennial/tsconfig.json
--- a/perennial/tsconfig.json	(revision ae515be16fbe8971a4f984879102ca2bc9bfd434)
+++ b/perennial/tsconfig.json	(date 1729200747548)
@@ -5,9 +5,6 @@
   "extends": "../chipper/tsconfig/shared/tsconfig-node.json",
   "include": [
     "js/**/*",
-    "test/**/*",
-    // TODO: this is horrible, MK would prefer to just ts-expect-error when importing instead. Let's talk https://github.com/phetsims/chipper/issues/1489
-    // TODO: Not only horrible, but it means that we lint chipper when we try to just lint perennial/ https://github.com/phetsims/chipper/issues/1489
-    "../chipper/js/grunt/lint.js"
+    "test/**/*"
   ]
 }
\ No newline at end of file
Index: chipper/js/grunt/Gruntfile.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/grunt/Gruntfile.js b/chipper/js/grunt/Gruntfile.js
--- a/chipper/js/grunt/Gruntfile.js	(revision cf1ba1ca3afc0e01765cba2de5bda62b96a8dd28)
+++ b/chipper/js/grunt/Gruntfile.js	(date 1729201417831)
@@ -44,13 +44,13 @@
    *
    * @param {string} task - The name of the task
    */
-  function registerPerennialTask( task ) {
-    grunt.registerTask( task, 'Run grunt --help in perennial to see documentation', () => {
-      grunt.log.writeln( '(Forwarding task to perennial)' );
+  function forwardToRepo( forwardingRepo, task ) {
+    grunt.registerTask( task, `Run grunt --help in ${forwardingRepo} to see documentation`, () => {
+      grunt.log.writeln( `(Forwarding task to ${forwardingRepo})` );
       const args = [ `--repo=${repo}`, ...process.argv.slice( 2 ) ];
 
       const isWindows = /^win/.test( process.platform );
-      gruntSpawn( grunt, isWindows ? 'grunt.cmd' : 'grunt', args, '../perennial', argsString => {
+      gruntSpawn( grunt, isWindows ? 'grunt.cmd' : 'grunt', args, `../${forwardingRepo}`, argsString => {
         grunt.log.debug( `running grunt ${argsString} in ../${repo}` );
       } );
     } );
@@ -74,8 +74,14 @@
     'production',
     'prototype',
     'create-sim',
-    'lint-everything',
+    'lint-everything', // on mixed shas
     'generate-data',
     'release-branch-list'
-  ].forEach( registerPerennialTask );
+  ].forEach( task => forwardToRepo( 'perennial', task ) );
+
+  [
+    'lint',
+    'lint-all'
+  // ].forEach( task => forwardToRepo( 'perennial-alias', task ) );
+  ].forEach( task => forwardToRepo( 'perennial', task ) );
 };
\ No newline at end of file
Index: quake/js/grunt/tasks/default.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/quake/js/grunt/tasks/default.ts b/quake/js/grunt/tasks/default.ts
--- a/quake/js/grunt/tasks/default.ts	(revision 5e65162ca567839cbd0429262e7d9324a5e8adf2)
+++ b/quake/js/grunt/tasks/default.ts	(date 1729201839987)
@@ -14,7 +14,7 @@
  */
 
 ( async () => {
-  await ( await import( '../../../../chipper/js/grunt/tasks/lint.ts' ) ).lintTask;
+  await ( await import( '../../../../perennial-alias/js/grunt/tasks/lint.ts' ) ).lintTask;
 
   await import( './build.ts' );
 
Index: chipper/js/scripts/hook-pre-commit-task.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/scripts/hook-pre-commit-task.js b/chipper/js/scripts/hook-pre-commit-task.js
--- a/chipper/js/scripts/hook-pre-commit-task.js	(revision cf1ba1ca3afc0e01765cba2de5bda62b96a8dd28)
+++ b/chipper/js/scripts/hook-pre-commit-task.js	(date 1729200737478)
@@ -16,7 +16,7 @@
 const generatePhetioMacroAPI = require( '../phet-io/generatePhetioMacroAPI' );
 const CacheLayer = require( '../../../chipper/js/common/CacheLayer' );
 const phetioCompareAPISets = require( '../phet-io/phetioCompareAPISets' );
-const lint = require( '../../../chipper/js/grunt/lint' );
+const lint = require( '../../../perennial-alias/js/grunt/lint' );
 const reportMedia = require( '../../../chipper/js/grunt/reportMedia' );
 const puppeteerQUnit = require( '../../../aqua/js/local/puppeteerQUnit' );
 const Transpiler = require( '../../../chipper/js/common/Transpiler' );
Index: perennial/js/grunt/tasks/lint-everything.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial/js/grunt/tasks/lint-everything.ts b/perennial/js/grunt/tasks/lint-everything.ts
--- a/perennial/js/grunt/tasks/lint-everything.ts	(revision ae515be16fbe8971a4f984879102ca2bc9bfd434)
+++ b/perennial/js/grunt/tasks/lint-everything.ts	(date 1729200816807)
@@ -8,6 +8,7 @@
 import grunt from 'grunt';
 import getDataFile from '../../common/getDataFile';
 import getOption from './util/getOption';
+import lint from '../lint.js';
 
 ( async () => {
 
@@ -18,15 +19,6 @@
   const chipAway = getOption( 'chip-away' );
   const showProgressBar = !getOption( 'hide-progress-bar' );
 
-  let lint;
-  try {
-    lint = ( await import( '../../../../chipper/js/grunt/lint.js' ) ).default;
-  }
-  catch( e ) {
-    console.log( 'lint process not found, is your chipper repo up to date?' );
-    lint;
-  }
-
   // The APIs are the same for these two versions of lint support
   if ( lint && ( lint.chipperAPIVersion === 'promisesPerRepo1' || lint.chipperAPIVersion === 'npx' ) ) {
     const lintReturnValue = await lint( activeRepos, {
Index: perennial/js/grunt/tasks/lint.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial/js/grunt/tasks/lint.ts b/perennial/js/grunt/tasks/lint.ts
--- a/perennial/js/grunt/tasks/lint.ts	(revision ae515be16fbe8971a4f984879102ca2bc9bfd434)
+++ b/perennial/js/grunt/tasks/lint.ts	(date 1729201389592)
@@ -1,29 +1,35 @@
-// Copyright 2024, University of Colorado Boulder
+// Copyright 2013-2024, University of Colorado Boulder
 
 /**
- * Lints this repository only
- *
- * @author Michael Kauzmann (PhET Interactive Simulations)
+ * lint js files. Options:
+ * --disable-eslint-cache: cache will not be read from, and cache will be cleared for next run.
+ * --fix: autofixable changes will be written to disk
+ * --chip-away: output a list of responsible devs for each repo with lint problems
+ * --repos: comma separated list of repos to lint in addition to the repo from running
  *
+ * @author Sam Reid (PhET Interactive Simulations)
  */
-import assert from 'assert';
-import grunt from 'grunt';
-import execute from '../../common/execute';
-import gruntCommand from '../../common/gruntCommand';
-import getOption from './util/getOption.ts';
+import * as grunt from 'grunt';
+import getOption from './util/getOption.js';
+import getRepo from './util/getRepo.js';
+import lint from '../lint.js';
 
-( async () => {
+const repo = getRepo();
 
-  const index = process.argv.indexOf( 'lint' );
-  assert( index >= 0, 'lint command does not appear' );
-  const tail = process.argv.slice( index + 1 );
+export const lintTask = ( async () => {
+  const cache = !getOption( 'disable-eslint-cache' );
+  const fix = getOption( 'fix' );
+  const chipAway = getOption( 'chip-away' );
 
-  // Prevent from also linting the chipper repo
-  if ( !getOption( 'repo' ) ) {
-    tail.push( '--repo=perennial' );
-  }
+  const extraRepos = getOption( 'repos' ) ? getOption( 'repos' ).split( ',' ) : [];
+
+  const lintReturnValue = await lint( [ repo, ...extraRepos ], {
+    cache: cache,
+    fix: fix,
+    chipAway: chipAway
+  } );
 
-  // Forward to chipper, supporting all of the options
-  // @ts-expect-error, remove in https://github.com/phetsims/perennial/issues/369
-  grunt.log.writeln( ( await execute( gruntCommand, [ 'lint', ...tail ], '../chipper', { errors: 'resolve' } ) ).stdout );
+  if ( !lintReturnValue.ok ) {
+    grunt.fail.fatal( 'Lint failed' );
+  }
 } )();
\ No newline at end of file

@samreid
Copy link
Member

samreid commented Oct 17, 2024

I pushed the results of these "history moves":

node perennial/js/scripts/copy-history-to-different-repo chipper/js/common/showCommandLineProgress.js perennial
node perennial/js/scripts/copy-history-to-different-repo chipper/js/grunt/lint.js perennial

We still need to deal with:

@zepumph zepumph assigned zepumph and unassigned samreid Oct 18, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member Author

zepumph commented Oct 18, 2024

  • Tell everyone to manually delete chipper/eslint/cache/ because it was moved to chipper dist. (and remove from .gitignore)

@zepumph
Copy link
Member Author

zepumph commented Oct 18, 2024

  • Factor out logic for getting linting options (duplicated in lint /all/everything)

zepumph added a commit to phetsims/perennial that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/perennial that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/perennial that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member Author

zepumph commented Oct 18, 2024

Converting lint.js to typescript now.

@zepumph zepumph assigned samreid and unassigned zepumph Oct 18, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 18, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
samreid added a commit to phetsims/perennial that referenced this issue Oct 21, 2024
@samreid
Copy link
Member

samreid commented Oct 21, 2024

@zepumph and I reviewed and everything looks good. One more TODO and other things in side issues.

@samreid samreid removed their assignment Oct 21, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 22, 2024
zepumph added a commit that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/quake that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 23, 2024
@zepumph
Copy link
Member Author

zepumph commented Oct 23, 2024

One last todo:

TODO: Move [parseLintOptions] to the lint module directly. #1489

@zepumph zepumph self-assigned this Oct 23, 2024
zepumph added a commit to phetsims/aqua that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/aqua that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 23, 2024
samreid pushed a commit that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 23, 2024
zepumph added a commit that referenced this issue Oct 23, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 23, 2024
@zepumph zepumph closed this as completed Oct 23, 2024
samreid pushed a commit that referenced this issue Nov 22, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
samreid pushed a commit that referenced this issue Nov 22, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
samreid pushed a commit that referenced this issue Nov 22, 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

2 participants