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

Use different names for tasks and modules #375

Open
zepumph opened this issue Oct 21, 2024 · 6 comments
Open

Use different names for tasks and modules #375

zepumph opened this issue Oct 21, 2024 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Oct 21, 2024

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
Copy link
Member Author

zepumph commented Nov 7, 2024

  • One step to do here is look through all modules at top level of js/grunt/ and if the only usage is the task, then inline it.

@samreid samreid self-assigned this Nov 8, 2024
@samreid
Copy link
Member

samreid commented Nov 8, 2024

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?

@samreid samreid assigned zepumph and unassigned samreid Nov 8, 2024
@zepumph
Copy link
Member Author

zepumph commented Nov 13, 2024

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
Copy link
Member

samreid commented Nov 13, 2024

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

  • checkoutMainAll
  • cherryPick
  • createOneOff
  • createSim
  • generateData
  • shaCheck

@zepumph
Copy link
Member Author

zepumph commented Nov 13, 2024

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).

samreid added a commit that referenced this issue Nov 13, 2024
samreid added a commit that referenced this issue Nov 13, 2024
samreid added a commit that referenced this issue Nov 13, 2024
samreid added a commit that referenced this issue Nov 13, 2024
@samreid samreid closed this as completed Nov 13, 2024
@zepumph
Copy link
Member Author

zepumph commented Nov 15, 2024

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

export const lintTask = ( async () => {

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?

@zepumph zepumph reopened this Nov 15, 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