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

Use arrow functions instead instead of normal ones #2198

Closed
aviral243 opened this issue Apr 4, 2020 · 33 comments
Closed

Use arrow functions instead instead of normal ones #2198

aviral243 opened this issue Apr 4, 2020 · 33 comments

Comments

@aviral243
Copy link
Member

As we move towards using let instead of var, we can also make use of another ES6 Javascript feature; Arrow functions. The benefits of using it will be:

  • Major benefit would be getting rid of this binding.
    e.g We come across instances of such format in our code where we need to bind this to use in surrounding code:

Screenshot from 2020-04-04 06-43-54

Arrow functions do that automatically.

  • Allows implicit return in case there is no body of a function.

e.g. An arrow function with no parameters and no body
let func = () => 1;

  • Syntax is cleaner than the old one.
    e.g. Functions with one parameter can also be defined without parantheses

let func = x => { return 2*x; }

Additional Context: I am working on this.

@quozl
Copy link

quozl commented Apr 4, 2020

I approve. ES6 was finalised in June 2015. This along with the other changes relating to ES6 moves Music Blocks beyond compatibility with most instances of Sugar. I'm fine with that, as it was never released for older platforms. I'll make sure to advise against trying it.

@aviral243
Copy link
Member Author

@quozl Thanks for approving the change.
I don't know much about the compatibility at this instant to comment on that, but it's good that new issues aren't popping up.

@aviral243
Copy link
Member Author

aviral243 commented Apr 4, 2020

Let's also have a task list to keep track:

  • js/
  • js/widgets/
  • js/blocks/
  • js/utils
  • planet/

@walterbender
Copy link
Member

Re James' comment, I had been pretty diligent about keeping E6 out of MB for a long time, but during GCI, I had let down my guard, so that bridge has already been crossed.

@walterbender
Copy link
Member

Still quite a few places we should make these changes.

@thapar-vansh
Copy link

Hey , if this is still open . I want to contribute.

@walterbender
Copy link
Member

Please make a pull request.

@thapar-vansh
Copy link

It says I can't commit since I don't have access to musicblocks

@walterbender
Copy link
Member

You cannot make a PR? Or you cannot merge your PR?

@quozl
Copy link

quozl commented Jan 22, 2023

Sounds like forgot to fork.

@guru9607
Copy link

I would love to contribute, can you assign it me?

@walterbender
Copy link
Member

We don't assign issues, but please feel free to make a pull requet.

Anshul-Ji added a commit to Anshul-Ji/musicblocks that referenced this issue Jan 29, 2023
Changed existing function syntax to match ES6 arrow function syntax
walterbender pushed a commit that referenced this issue Jan 29, 2023
Changed existing function syntax to match ES6 arrow function syntax
@RiteshSinha919
Copy link
Contributor

@walterbender can you review my pull request, I have added it just a moment ago. Let me know if there are any changes needed

Anshul-Ji added a commit to Anshul-Ji/musicblocks that referenced this issue Jan 30, 2023
Changed existing anonymous function syntax to match ES6 arrow function syntax
@simplysabir
Copy link

Hey, Can I work on this issue?

@quozl
Copy link

quozl commented Jan 30, 2023

@simplysabir we don't assign issues, but please feel free to make a pull request. You should have seen that said already in this issue. If you have seen somewhere that says you have to ask before fixing a problem, please point it out to us so we can correct it!

We like to merge the best possible pull request.

Anshul-Ji added a commit to Anshul-Ji/musicblocks that referenced this issue Jan 30, 2023
*Changed existing anonymous function syntax to match ES6 arrow function syntax

*Got rid of `this` binding in some files
Anshul-Ji added a commit to Anshul-Ji/musicblocks that referenced this issue Jan 30, 2023
* Changed existing anonymous function syntax to match ES6 arrow function syntax

* Got rid of `this` binding in some files
walterbender pushed a commit that referenced this issue Jan 30, 2023
Changed existing anonymous function syntax to match ES6 arrow function syntax
walterbender pushed a commit that referenced this issue Jan 31, 2023
* Changed existing anonymous function syntax to match ES6 arrow function syntax

* Got rid of `this` binding in some files
@RiteshSinha919
Copy link
Contributor

@walterbender is there any need to change the /js as mentioned by @aviral243. He already did these changes but didn't merge it, rather he closed the pull request.

@walterbender
Copy link
Member

I think he is going to reopen with just the changes to /js and not the other changes.

@RiteshSinha919
Copy link
Contributor

RiteshSinha919 commented Jan 31, 2023

@walterbender Can I do this for a time being ? I think /js is the last file to be updated. We can close this issue with that being done

@walterbender
Copy link
Member

It would be much more useful if you'd work on one of the "good first" issues with more substance.

@Vivekk-11
Copy link

Hey, I'm working at this issue right now, is that all right?

@walterbender
Copy link
Member

Looking forward to seeing the PR

@Snach13
Copy link

Snach13 commented Feb 4, 2023

I'm converting normal functions into arrow functions

  • (js/abc.js)
  • [] (js/activity.js) - this file is too big so I don't think all the functions converted to the arrow
  • (js/basicblocks.js)

and working forward !!

@Snach13
Copy link

Snach13 commented Feb 4, 2023

  • (js/block.js ) file checked
  • (js/lilypond.js) file checked
  • (js/macros.js) file checked
  • (js/mxml.js) file checked
  • (js/pallete.js) file checked
  • (js/piemenus.js) file checked

Snach13 added a commit to Snach13/musicblocks that referenced this issue Feb 4, 2023
Vivekk-11 added a commit to Vivekk-11/Vivek-musicblocks that referenced this issue Feb 5, 2023
@Husain01
Copy link

Husain01 commented Feb 6, 2023

I see the issues aren't being assigned to anyone. Can I start working on it too?

@Vivekk-11
Copy link

Hey @walterbender, I've made a Pull Request. Could you see it, please?

@quozl
Copy link

quozl commented Feb 6, 2023

We support any number of pull requests. When working closely with others, and the changes are likely to be merged (as in this case because they are straightforward), it can help if you rebase from each other's pull request branches. That means the maintainer may need to only merge one pull request instead of several.

If there's something about the other person's commits that mean you won't rebase or cherry-pick them, then that means you really ought to talk to them about it in their pull request.

@Vivekk-11
Copy link

Now should I just rebase from the other person's pull request branch?

@singhshivansh244
Copy link
Contributor

I have solved this issue just look my Pull Request. Please

@quozl
Copy link

quozl commented Feb 6, 2023

Thanks. It's great that so many people are helping. If you are waiting for review, please take a moment to review one of the other pull requests on the same issue.

The recent pull requests relating to this issue are;

You might detect mistakes, accidental changes, help maintain consistency and code quality.

You might test the changes to see if they work for you; if so, post a comment to say what you tested. Be sure to test MusicBlocks first without the changes, we don't want to hear about your problems testing that aren't related to the pull requests.

See our Guide for Reviewers.

Vivekk-11 added a commit to Vivekk-11/Vivek-musicblocks that referenced this issue Feb 7, 2023
Vivekk-11 added a commit to Vivekk-11/Vivek-musicblocks that referenced this issue Feb 7, 2023
@hirentimbadiya
Copy link

i am interested to work on this issue , can i start ? @quozl

@walterbender
Copy link
Member

@hirentimbadiya please read through the issue comment thread. You will observe that (1) we don't assign issues but we do accept PRs and (2) many others are already working on this issue. I would recommend you tackle a different one (there are many marked "good first issue".

@quozl
Copy link

quozl commented Feb 8, 2023

@hirentimbadiya, I agree with @walterbender. We do not assign issues. You should do whatever work you choose and make a pull request, or if there are pull requests already open help others with them. Given how many people are working on this issue now, you may find it quite difficult to engage with it, but if working with other people is your speciality, get involved in their work. 😁

@walterbender
Copy link
Member

I think this is well covered now. Thanks to everyone who contributed to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.