Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix #1098: Some code uses $.each, some uses Array.prototype.forEach #3087

Merged
merged 3 commits into from
Mar 14, 2013
Merged

Fix #1098: Some code uses $.each, some uses Array.prototype.forEach #3087

merged 3 commits into from
Mar 14, 2013

Conversation

TomMalbran
Copy link
Contributor

This fixes #1098 by replacing every $.each in the core code with either Array.forEach for arrays or CollectionUtils.forEach for objects. The remaining $.each are only in jQuery.

@pthiess
Copy link
Contributor

pthiess commented Mar 12, 2013

Community pull request - open to anyone to grab.

@WebsiteDeveloper
Copy link
Contributor

i'll look at it

@peterflynn
Copy link
Member

@WebsiteDeveloper just to be clear, we will need a code review from a Brackets committer also before landing this. But still feel free to review it -- an extra set of eyes to point out issues never hurts!

@ghost ghost assigned jbalsas Mar 12, 2013
@jbalsas
Copy link
Contributor

jbalsas commented Mar 12, 2013

Assigning to myself...

@WebsiteDeveloper, I'll be tagging along with you for this one. Please, feel free to review and voice your concerns, if any. I'll go over it myself later today or tomorrow and do the same, and then we'll iterate until everything looks good before merging, as usual ;)

@WebsiteDeveloper
Copy link
Contributor

@peterflynn i did know that, but as you said i'll just look at it
@jbalsas No issues spotted, all unit tests pass so should be fine.
After all, those are simple changes.

@TomMalbran
Copy link
Contributor Author

Simple, but could break anything if using the wrong forEach. Anyway I did tested everything and run the tests before submitting it.

There are still a couple of Object.keys().forEach iterations left. Those are OK, but for better consistency, should these be CollectionUtils.forEach loops?

@WebsiteDeveloper
Copy link
Contributor

As i'm a fan of consistency i would say yes, because generally consistency makes i a lot easier for new developers to understand the code, also it makes the code more maintainable.

@jbalsas
Copy link
Contributor

jbalsas commented Mar 13, 2013

@TomMalbran I think we only have Object.keys().forEach in PreferencesManager and UrlParams. I'd also prefer to move them over to CollectionUtils.forEach for consistency.

Other than that, this is good to go.

@TomMalbran
Copy link
Contributor Author

@jbalsas Yes, those where the ones I was referring too, since I already replaced the Strings ones. I replaced those too. Should be good to go :)

@jbalsas
Copy link
Contributor

jbalsas commented Mar 14, 2013

@WebsiteDeveloper Thanks for your help with the review! Keep your eyes open, surely more like this are on their way ;)
@TomMalbran Sorry I couldn't get to this yesterday...

Looks good to me. Merging.

jbalsas added a commit that referenced this pull request Mar 14, 2013
Fix #1098: Some code uses $.each, some uses Array.prototype.forEach
@jbalsas jbalsas merged commit 4caf66d into adobe:master Mar 14, 2013
@jbalsas
Copy link
Contributor

jbalsas commented Mar 14, 2013

@TomMalbran By the way, I just realized that you're still using your personal fork of Brackets to create your pull requests. I think should be able to create branches directly in this main repo if that's an easier workflow for you.

@TomMalbran TomMalbran deleted the tom/fix-issue-1098 branch March 15, 2013 00:34
@TomMalbran
Copy link
Contributor Author

Right. I think I am just used to do it like this, and since I am somehow still new to git I would need to learn how to do it.

And thanks for the review.

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

Successfully merging this pull request may close these issues.

Some code uses $.each, some uses Array.prototype.forEach
5 participants