phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
http://scenerystack.org/
MIT License
4 stars 5 forks source link

Use different names for tasks and modules #375

Open zepumph opened 3 weeks ago

zepumph commented 3 weeks ago

I have been getting pretty confused quite often working in perennial grunt tasks. For example: lint.ts lives in perennial. and is a module in js/grunt/ as well as a task in js/grunt/tasks/. This means that when I search for lint, I see this in webstorm:

image

Maybe we can keep the tasks name the same, and have a convention for what the module is called:

lintModule linter (checker, modulifier, etc)

@samreid is curious if we can reuse the same file.

zepumph commented 1 week ago
samreid commented 1 week ago

The first problematic occurrence in chipper is generate-a11y-view-html.ts which calls generateA11yViewHTML.ts and has 2x usage sites so cannot be inlined.

I think the names are reasonable here since the camel casing differentiates. My only other idea about how this could be better is to do like python's if __name__ == '__main__'. In Node.js I believe it would be something like:

if (require.main === module) {
  ...
}

We have previously decided against this, but it could solve the proliferation of files problem. But I think we also added other lint rules to prevent from importing from grunt/tasks, which we would have to undo/rethink. @zepumph what do you advise?

zepumph commented 3 days ago

I totally agree with you about the above. I believe that all that is left here is to audit grunt current grunt tasks for any others that do not need the module, and that can indeed be appropriately inlined without much risk that someone is going to want to use it as a module in the future. Note that this is worthy because many grunt tasks modules were created to keep too much logic out of the top level giant grunt file (very reasonable). This is not longer needed.

For example, it was nice to not need a module in reopen-issues-from-todos, where it was obvious that the 100 lines of code were just used as a module to keep them out of the perennial gruntfile. (only slightly moot now that it is a script and not a grunt task).

Are there any more like this?

samreid commented 2 days ago

I finished going through the chipper tasks. In perennial, we can likely inline:

zepumph commented 2 days ago

I think we should inline all the above except createOneOff and createSim, since having them next to createRelease seems nicer, and it is a bit arbitrary that they aren't used by a module but createRelease is (I mean to say that it could easily change in the future).

zepumph commented 1 day ago

I see two patterns that I think would be good to align:

Sometimes we export the task promise named moduleTask, and somtimes it is just module

https://github.com/phetsims/perennial/blob/8ed959fbbba5741f5b5100adca92dd17fc19ad5e/js/grunt/tasks/lint.ts#L18

https://github.com/phetsims/chipper/blob/029ea52/js/grunt/tasks/report-media.ts#L18

Can we determine which way is best. Also, could we just export default instead of naming it?