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

Review modulify.js #945

Closed
12 tasks done
Denz1994 opened this issue May 14, 2020 · 2 comments
Closed
12 tasks done

Review modulify.js #945

Denz1994 opened this issue May 14, 2020 · 2 comments
Assignees

Comments

@Denz1994
Copy link
Contributor

Denz1994 commented May 14, 2020

From #872:

modulify.js

  • File export statement is not using ES6 arrow function
  • modulifyFile and modulifyMipmap are handled asynchronously. I would suggest modulifyImage to be handled asynchronously as well, for consistency with these other functions. I'll leave to your discretion.
  • Consider inlining const x = loadFileAsDataURI( abspath ); with its usage in base64:${x}. Or at least consider renaming to fileDataURI.
  • modulifyMipmap uses an options object to pass in args for createMipmap()'. The args aren't noted as optional. Consider renaming to config`.
  • In expandDots consider renaming x to parentDirectory maybe?

Docs:

  • expandDots is missing js doc
  • convertSuffix' is missing @return` doc
  • getSuffix is missing @return doc
  • Typo in export statment docs? Should read * @param {string} repo - the name of a repo, such as 'joist'
  • grunt --help reads modulify will generate js files for images/strings/audio/etc while the top level documentation mentions "images or sounds". Should the files mentioned be consistent?

Miscellaneous:

  • TODO: Related to support for non-defaults exports. Has this been addressed?
  • TODO: Related to mipmap support. This file currently supports modifying mipmaps so can this be removed?

Assigning to @jonathanolson to respond to the above comments.

@jonathanolson
Copy link
Contributor

I believe all of the above should be handled with the above commit, can you verify? (Good review items, thanks!)

@samreid samreid assigned samreid and unassigned Denz1994 Jan 28, 2021
@zepumph
Copy link
Member

zepumph commented Jan 28, 2021

I think this is probably good to go. Closing, but please reopen @samreid or @jonathanolson if you think there is more here.

@zepumph zepumph closed this as completed Jan 28, 2021
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

4 participants