-
Notifications
You must be signed in to change notification settings - Fork 823
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
utils/musicutils.js: Pretiffy,linting and JSDoc Documentation #2814
Conversation
I have restructured the code as there were many errors of the type: Please Review @meganindya. |
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.
cherry pick your commits, 14 commits for just one file is too much, I can see that at times you have even repeated your commit messages.
Slice them down a little bit, do a rebase.
It's easier to review if commits are clustered and not blown out of proportions into 2 digit numbers for just one file.
Thanks for the review @ricknjacky... |
A paraphrase :- Some (non-exhaustive) benefits of using these are syntax highlighting, warning/error annotations, formatting, auto-refactoring, tons of customizable keyboard shortcuts, etc. Not a hard and fast rule, it's your call on which editor you chose to use as extensions are available in almost all of them, just a suggestion VSCode is best alternative out there. |
I already use VSCode and had also added the ESLint extension but didn't know why it wasn't working. I also saw that the .eslintrc file was already present in our project repo. |
Gotta enable it ... right bottom corner |
Thanks @meganindya ..... got it. |
This is huuuge |
Yeah... and that's why it has this many linting errors and so many commits..... |
Pls run down what you've done. I can't follow. |
Yeah..so what I did was firstly Pretiffied the file and added JSDoc documentation and removed the debug logs. Then when I pushed these commits,
Since there were a huge number of functions in this file, It took a lot of commits to resolve all of them. |
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.
you can close this PR,
I recently dealt with this file and it is linted, prettified etc (:
Issue Reference: #2767, #2630, #2609