pre-commit code updates

phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out

http://scenerystack.org/

MIT License

5 stars 5 forks source link

pre-commit code updates #404

Open zepumph opened 2 weeks ago

zepumph commented 2 weeks ago

From https://github.com/phetsims/perennial/issues/370

[x] Move this code to ~perennial-alias~ chipper/js/grunt/tasks and use getOption
[x] Rename hook-pre-commit.ts to pre-commit.ts. And the task took.
[x] Create a grunt task that runs pre commit hooks for a given repo or repos. Run it via the ts script, not the bash launcher.
[x] Grunt task to support the "all working copy changes repos" (--changed) mode.
[x] hook-pre-commit.sh calls sage run grunt/tasks/pre-commit.ts because the script moves to grunt.
[x] grunt options can override the defaults from build-local.json. Probably something like grunt pre-commit --lint=true --report-media=true --check=true --test=true.
[x] Remove --forceTasks added in https://github.com/phetsims/chipper/issues/1350#issuecomment-2452672149
[x] Rename phet-io-api-compare -> phet-io-api Don't forget about build-json key and ping devs.

samreid commented 1 week ago

The precommit hook implementation is currently:

../perennial-alias/bin/sage run ../chipper/js/scripts/hook-pre-commit.js

should we change it to (0.3s faster on average)

../perennial-alias/bin/sage run ../perennial-alias/js/grunt/tasks/pre-commit.js

or this (stable API, cleaner, but assumes grunt is installed and is 0.3s slower on average)

grunt pre-commit

There is also a guard:

# Only run pre-commit hooks in main, see https://github.com/phetsims/perennial/issues/276

But maybe a better guard would be: if grunt is installed and it has a pre-commit task. Or alteratively, if sage exists and the pre-commit task exists.

zepumph commented 1 week ago

I think we should keep it at ../perennial-alias/bin/sage run ../perennial-alias/js/grunt/tasks/pre-commit.js.

zepumph commented 4 days ago

We hit this over in https://github.com/phetsims/perennial/issues/410, where we need to reinstall all git hooks because we renamed pre-commit.js -> .ts.

[x] So we are adapting things a bit so that the pre-commit file just points to another bash file. This way that entrypoint can be changed without the need to reinstall all git hooks.

samreid commented 2 days ago

Move this code to perennial-alias

Partway through this, I noticed these chipper files are used in this process. I don't believe that perennial can depend on chipper, and I don't believe we should move these behaviors to perennial:

import CacheLayer from '../../../chipper/js/common/CacheLayer.js'; import reportMedia from '../../../chipper/js/grunt/reportMedia.js'; import transpile from '../common/transpile.js'; import getPhetLibs from '../grunt/getPhetLibs.js'; import generatePhetioMacroAPI from '../phet-io/generatePhetioMacroAPI.js'; import phetioCompareAPISets from '../phet-io/phetioCompareAPISets.js';

@zepumph please advise

samreid commented 1 day ago

Implemented, seems to be working nicely. Would be good to review somewhat carefully. Changes are in a branch pre-commit-chipper-404 in perennial-alias and chipper.