search

phetsims / chipper

Tools for developing and building PhET interactive simulations.
MIT License
11 stars 12 forks source link

`grunt modulify` should run reportMedia #1412

Open pixelzoom opened 5 months ago

pixelzoom commented 5 months ago

Noted over in https://github.com/phetsims/chipper/issues/1411#issuecomment-1847533479 ...

reportMedia.js checks the license files. It looks like grunt modulify is not calling reportMedia, and that is instead deferred until grunt build. That seems way too late -- missing licenses should be reported when the image is added, which is when grunt modulify is run.

So... Should grunt modulify be running reportMedia? Assigning to the authors of modulify.js.

samreid commented 5 months ago

Running reportMedia after grunt modulify sounds reasonable to me, but I want to also note reportMedia also runs in the precommit hooks which is closer to the "when the image is added" phase. So I don't feel too strongly.

jonathanolson commented 5 months ago
zepumph commented 5 months ago

Ok. So when I went to actually work on this, I actually ended up not agreeing with @jonathanolson here. I was thinking about prototyping assets, and how it could be annoying to have to have the license correct BEFORE even being able to test the sim. I'm really happy to be outvoted here, but perhaps it is enough that there is an error when running the command.

@jonathanolson what do you think?

zepumph commented 5 months ago

Also, I wasn't clear if your recommendation was to integrate report media logic into Modulify so that only the assets with licenses are modulified. Personally I think that is suave but overkill and likely a fair bit of work. It's just as easy to all or nothing it. I think people want to do the right thing, they just need the reminder.