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

Adding classes #2633

Conversation

kushal-khare-official
Copy link
Contributor

@kushal-khare-official kushal-khare-official commented Nov 28, 2020

Issue #2629
Port to class definition and more:

  • block.js
  • blockfactory.js
  • blocks.js
  • boundary.js
  • languagebox.js
  • palette.js
  • pastebox.js
  • pluginsviewer.js
  • protoblocks.js
  • saveInterface.js
  • toolbar.js
  • trash.js
  • widgets/help.js
  • widgets/meterwidget.js
  • widgets/modewidget.js
  • widgets/musickeyboard.js
  • widgets/oscilliscope.js
  • widgets/phrasemaker.js (rename class from PitchTimeMatrix to PhraseMaker)
  • widgets/pitchdrummatrix.js
  • widgets/pitchslider.js
  • widgets/pitchstaircase.js
  • widgets/rhythmruler.js
  • widgets/statistics.js
  • widgets/status.js
  • widgets/temperament.js
  • widgets/tempo.js
  • widgets/timbre.js
  • widgets/widgetWindows.js

Reviewers:

@meganindya

@meganindya
Copy link
Member

meganindya commented Nov 28, 2020

Three things:

  • do a rebase every time you start with (don't want previous commits reapprearing)
  • (for this one) one file at a time please
  • mark PR as draft if incomplete

@kushal-khare-official kushal-khare-official marked this pull request as draft November 28, 2020 11:46
@kushal-khare-official
Copy link
Contributor Author

@meganindya @walterbender I was editing pluginsviwer.js. Could you provide a plugin for testing purposes.

@meganindya
Copy link
Member

Three things:

  • do a rebase every time you start with (don't want previous commits reappearing)
  • (for this one) one file at a time please
  • mark PR as draft if incomplete

11k+ lines will be a mammoth task to review.

Please follow the following:

  1. stash any uncommitted changes
  2. rebase you master to current master of sugarlabs/musicblocks
  3. create a new branch
  4. cherry pick your commits on one file
  5. create a new PR from that branch
  6. wait for review
  7. make any suggested changes if at all
  8. go back to 2 until all the files have been dealt with one at a time
  9. pop stash and practice the same thereafter

@meganindya
Copy link
Member

it's "editing" btw

@walterbender
Copy link
Member

walterbender commented Nov 28, 2020 via email

@kushal-khare-official
Copy link
Contributor Author

kushal-khare-official commented Nov 29, 2020

11k+ lines will be a mammoth task to review.

@meganindya though its says 11k+ lines, its is just because creating a class indents the whole code by one tab space. It cant be dodged.

@kushal-khare-official
Copy link
Contributor Author

@meganindya To simplify the reviewing process, I was thinking maybe I should reduce this one tab space added to the all the files. Though the files will not be correctly indented, it will simplify the review process and then we can add this tab space later when we lint all the files

@meganindya
Copy link
Member

You are literally spamming. I told you to break this into multiple PRs and open one at a time, not all 25 at once.

Please close all your PRs except the first one (not this one, the one after this). We can reopen them one by one as required. In fact, close this one too, this is now redundant.

It's getting nearly impossible for us to follow anything. This sort of change maybe simple, but due to the scattered nature, needs to be verified thoroughly.

If you do not follow, we might have no other way out other than taking down your access.

@kushal-khare-official
Copy link
Contributor Author

You are literally spamming. I told you to break this into multiple PRs and open one at a time, not all 25 at once.

Sorry for that. I have closed rest of the PRs.

@meganindya meganindya closed this Dec 1, 2020
@kushal-khare-official kushal-khare-official deleted the adding-classes branch December 1, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants