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

js/turtleactions: refactor, lint & prettify #2813

Closed
wants to merge 3 commits into from

Conversation

ricknjacky
Copy link
Member

issue references: #2609, #2767

@ricknjacky
Copy link
Member Author

@meganindya pls review this.

Copy link
Member

@ksraj123 ksraj123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more helpful if we had where the globals come from in the same file. I think the reason behind enabling linting was that we could identify and fix other issues in the process. I guess, fixing linting issues in a way that just make the errors go away is as good as not having linting at all. Just my humble opinion. Thanks

js/turtleactions/IntervalsActions.js Outdated Show resolved Hide resolved
js/turtleactions/MeterActions.js Outdated Show resolved Hide resolved
js/turtleactions/ToneActions.js Outdated Show resolved Hide resolved
@ricknjacky
Copy link
Member Author

It would be more helpful if we had where the globals come from in the same file. I think the reason behind enabling linting was that we could identify and fix other issues in the process. I guess, fixing linting issues in a way that just make the errors go away is as good as not having linting at all. Just my humble opinion. Thanks

Totally agree. But that's not what I did.
With the nascent experience I have of working on the codebase, I have addressed your concerns.
Also,

Core of MusicBlocks resides in the /js, /blocks and the /turtleactions subdirectories. As a result there are many globals in these directories, js/turtleactions in this case. There are architectural issues pertaining to this as well, hopefully in MB2 we can obviate this instead of having this galore of GOD objects engendering regressions and all sorts of other issues.

Thanks a lot for the review btw

@ksraj123
Copy link
Member

ksraj123 commented Feb 7, 2021

@meganindya Please go through my comments in my review which have been marked as resolved. I was working on some other issue that involved one of the files in this PR so I thought reviewing this would be a good opportunity to learn. Thanks

@ricknjacky
Copy link
Member Author

ricknjacky commented Feb 7, 2021

@ksraj123 Thx for pointing out your concerns. I have unresolved your comments on the review, waiting for better answers to these. Surely it's a great opportunity to learn more about how the codebase is inter-connected.

Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the locations too

/*
   Global locations
    path..
        .., .., ...
 */

js/turtleactions/IntervalsActions.js Outdated Show resolved Hide resolved
js/turtleactions/MeterActions.js Outdated Show resolved Hide resolved
js/turtleactions/MeterActions.js Outdated Show resolved Hide resolved
js/turtleactions/ToneActions.js Outdated Show resolved Hide resolved
@ricknjacky
Copy link
Member Author

ricknjacky commented Feb 9, 2021

Thanks for pointing out my mistakes and sharing your feedback @meganindya

@ricknjacky
Copy link
Member Author

Where is args?

static defineMode(name, turtle, blk) {
            const tur = logo.turtles.ithTurtle(turtle);

            tur.singer.inDefineMode = true;
            tur.singer.defineMode = [];
            let modeName;
            if (name === null) {
                logo.errorMsg(NOINPUTERRORMSG, blk);
                modeName = "custom";
            } else {
                modeName = args[0].toLowerCase();
            }

@ricknjacky
Copy link
Member Author

blk must have been passed to the methods from corresponding classes in /blocks. If it isn't that's an error. Certainly not a global.

@meganindya
I have removed the argument blk and _blk accordingly.

Pls review and provide feedback, thanks.

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