fix: replaced the old script that was blocking p5.sound.js multiple files' docs generation#1225
Open
kunstewi wants to merge 2 commits intoprocessing:2.0from
Open
fix: replaced the old script that was blocking p5.sound.js multiple files' docs generation#1225kunstewi wants to merge 2 commits intoprocessing:2.0from
kunstewi wants to merge 2 commits intoprocessing:2.0from
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #1226 and #72 (p5.sound.js)
@ksen0 @limzykenneth please take a look whenever you are free.
Thanks a lot for your time.
Old Script
Problems
In the p5.sound.js repository, global library functions like loadSound(), getAudioContext(), and userStartAudio() are documented with the tag @for p5.sound. YUIDoc reads this information and assigns the functions to the p5.sound class. The old script added p5. to everything without thinking, which meant it took those methods and assigned them to a class called p5.p5.sound that does not exist.
The old script mistakenly added p5. to the front of every class and item name, regardless of the situation. This works for a class named SoundFile, which becomes p5.SoundFile. However, if a developer specifically wrote @Class p5.SoundFile in the JSDoc comments, the old script would incorrectly change it to p5.p5.SoundFile.
The old script used a for...in loop to iterate over the keys inside soundData.classes. In JavaScript, adding and deleting keys in an object while looping through it with a for...in loop is very risky. The JavaScript compiler may become confused. It could end up iterating over the newly added keys in an infinite loop, skipping existing keys, or generating errors.
Updated Script
this updated script fixes these three critical bugs:
The new script specifically hunts for anything assigned to the p5.sound class and explicitly reassigns it to the p5 class. This correctly integrates them as global methods in the documentation (alongside things like createCanvas() and ellipse()) instead of breaking them.
The new script safely double-checks if the string already starts with "p5.". If it does, it skips it; if it doesn't, it adds it. This prevents double-prefixing.
The new script uses Object.keys() to grab a strict, immutable array of the original class names before the loop starts. It then safely loops over that array, ensuring that any modifications made to soundData.classes down below have no effect on the loop's logic.
Results
Previously no result for userStartAudio or any methods in Utils.js and also no loadSound method from SoundFile
After the change all methods are present in the reference docs