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

Add format and format-verify tasks to grunt #994

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

NickCrews
Copy link
Contributor

This adds two grunt tasks, format and format-verify. The first reformats files for you, and the second just checks that files are formatted, eg for pre-commit hooks.

The src attribute, where the beautifuer looks for files, is wrong of course, but this is helpful for testing within the chipper repo. I still need to figure out how to make it so you can call this from any repo, since at this point when I do that I get an error because Local Npm module "grunt-jsbeautifier" not found. Do I need to add this to every repo's package.json? Somehow I doubt it, since those look really clean at this point...

This uses the .jsbeautifyrc file from https://github.com/phetsims/phet-info/blob/master/ide/sublime-text/.jsbeautifyrc, so this should follow the PHET conventions.

@NickCrews
Copy link
Contributor Author

This is a first stab at addressing phetsims/phet-info#150

@pixelzoom
Copy link
Contributor

Assigning to @ariel-phet to prioritize and assign.

@zepumph zepumph self-requested a review November 5, 2020 21:20
@zepumph zepumph assigned zepumph and unassigned ariel-phet Nov 5, 2020
`grunt format` now runs js-beautify and sortImports on all js files in a given repo (or the current one if none is given). In addition, you can use `grunt format --verify` to just check that the files are formatted, but not actually modify them. This could be useful for a pre-commit hook.

The `grunt format-all` variation is similar to lint-all in that it runs format on all the repos that are libraries of the supplied repo.
@NickCrews
Copy link
Contributor Author

Updated so you can run format and format-all as grunt tasks from any repo. After testing on a couple repos (twixt and energy-forms-and-changes) there were a couple differences. Indentation, import ordering (that comes from your existing grunt task sortImports though...), newlines were changed. I didn't actually like the end result, but it's palatable. Some tweaking still probably possible.

This also only does .js files, but presumably you may want something for all file types. Possibly unibeautify would be useful?

@zepumph zepumph marked this pull request as ready for review December 18, 2020 02:07
@zepumph zepumph merged commit 6887eb5 into phetsims:master Dec 18, 2020
@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 18, 2020

FYI... Merging this pull request in 6887eb5 introduced a new dependency on js-beautify (in format.js). This broke tasks like grunt lint. Everyone will need to do npm prune and npm update in their local chipper repo. I posted a PSA to the dev-public Slack channel.

This is the kind of breaking change that should probably be announced on the Google Group, but I haven't done that.

@zepumph
Copy link
Member

zepumph commented Dec 19, 2020

Sorry about that @pixelzoom! I will fix this while we are experimenting, and only add a dependency if we go down this road.

@NickCrews
Copy link
Contributor Author

Hey, thanks @zepumph for keeping the momentum going! This looks great. I'll add a few comments in phetsims/phet-info#150.

One thing though, is I notice that package-lock.json is not checked into this, or any phet repo. I'm an NPM newbie, but as I understand it package-lock.json should be checked in so that your dependency tree is always exactly reproducible. This possibly deserves its own discussion, but if this is a conscious choice by PHET then ignore me.

@pixelzoom
Copy link
Contributor

It might be worth discussing again, because I don't recall the discussion. But turning off package-lock.json was a conscious decision by PhET. And it's an explicit step in the "Getting Started" sections of the PhET Development Overview:

  1. Run npm config set save false so that package-lock.json files are not created.

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

Successfully merging this pull request may close these issues.

4 participants