-
Notifications
You must be signed in to change notification settings - Fork 24
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
update and cleanup contributing guidelines #73
Conversation
Co-authored-by: Daniel Shiffman <[email protected]>
@myrahsa this pull request is a draft of a "contributing" guide to the codebase of ml5.js that @ziyuan-linn is working on. This is something that aligns with the work you are proposing to do around a "style" or "language" guide (apologies if I am mis-stating this) for contributing documentation and other materials. Your work could be incorporated into this document, a separate document, or both! |
Thanks @shiffman! I'll start putting together my work throughout the week and see if this will work for my purposes. |
Hi @ziyuan-linn, apologies for losing track of this. I'd like to merge this wonderful additional documentation! Can you take a look at what is conflicted and let me know when I can review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ziyuan-linn thank you for working on this! I've made some light edits for clarity and style, take a look and let me know if you have questions! I'll merge after you do one more pass? Thank you!!
CONTRIBUTING.md
Outdated
let bodyPose = ml5.bodyPose({ modelType: "full" }, "blazepose"); | ||
``` | ||
|
||
8. Asynchronous ml5 functions should support the callback interface, and support the promise/async await interface as well when possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8. Asynchronous ml5 functions should support the callback interface, and support the promise/async await interface as well when possible. | |
8. Asynchronous ml5 functions should support the callback interface, and support the promise/async await interface as well when possible. |
Are we supporting promise/async/await with the models? maybe this should say something like. "The ml5.js models do not currently support promises (async/await), however, we are considering adding this in the future and plan to follow the p5.js's lead."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the current code does support the promise interface when possible. Perhaps we can say something along the lines of "Although some existing code has both callback and promise interface, the ml5.js models do not currently support promises (async/await) officially, however, we are considering adding this in the future and plan to follow the p5.js's lead."
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
Co-authored-by: Daniel Shiffman <[email protected]>
@shiffman Thank you for the review! I will take another look. Do you think it would make sense to break up the contributing guide into multiple documents? Perhaps we can have a guide for any contributor and a guide for maintainers/ ml5 devs. We could also break the API guideline into a separate document. The documents can also link to each other as additional resources. |
Let's finish and merge this first and we can think about breaking it up as a separate step? Also, I wonder what, if anything, from here should be on the website? Perhaps a link to here from the FAQ? cc @MOQN @QuinnHe Let me know when this is ready for one final review and merge! |
Yes, we have a dedicated page for contributing notes. We can update the content there accordingly! cc @MOQN |
Co-authored-by: Daniel Shiffman <[email protected]>
Hey all, this is looking great! I think I should merge this so it can be shared at today's meeting! We can make additional changes and updates in new branches. Thanks all! |
WIP guidelines for standardizing the API across different ml5 models