1103192 - [Messages] replace Utils.extend by Object.assign

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw]]

see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

We can now use Object.assign directly instead of implementing a custom function.

Comment 1

[William Lawson[:wlawson]]

I'd like to be assigned to this bug, could someone assign me please.

Comment 2

[Julien Wajsberg [:julienw]]

Hey William,

I'm glad you asked :)

You can find some startup information in https://github.com/julienw/gaia/blob/1082618-sms-readme/apps/sms/README.md, feel free to connect on IRC #gaia-messaging if you need anything!

Comment 3

[cvfergus]

Hello Julien and William,

I ran across this and thought it looked interesting and within my skill set (hopefully). Since the last comment here is three weeks old, I spent some time today getting set up and testing this patch as well as I'm able. I hope that's okay, I've never done any open source work before so I'm unsure of the etiquette.

Charley

Comment 4

[cvfergus]

Attachment: Patch, attempt 1.

Comment 5

[William Lawson[:wlawson]]

(In reply to cvfergus from comment #3)
> Hello Julien and William,
> 
> I ran across this and thought it looked interesting and within my skill set
> (hopefully). Since the last comment here is three weeks old, I spent some
> time today getting set up and testing this patch as well as I'm able. I hope
> that's okay, I've never done any open source work before so I'm unsure of
> the etiquette.
> 
> Charley

Hey Charely,

I think you can take this one, need to improve my coding skills a little more before I can progress onto contributing even to bugs such as this. Thanks for the message.

Comment 6

[Julien Wajsberg [:julienw]]

Thanks for the notice William; I don't know where you live, but it's probably easier to start as a beginner if you can join a Firefox OS hackathon somewhere :)

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8536913 [details] [review]
Patch, attempt 1.

I did a first look but didn't really look more than that, and I didn't test on the phone.

Oleg, can you take over this review? Especially if you can find other places where it could be appropriate to use Object.assign, please suggest :) I looked a EventDispatcher.mixin but I didn't find it was appropriate (because we're checking if the target exists).

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8536913 [details] [review]
Patch, attempt 1.

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Comment on attachment 8536913 [details] [review]
> Patch, attempt 1.
> 
> I did a first look but didn't really look more than that, and I didn't test
> on the phone.
Looks good to me, and works on device as well.

> Oleg, can you take over this review? Especially if you can find other places
> where it could be appropriate to use Object.assign, please suggest :) I
> looked a EventDispatcher.mixin but I didn't find it was appropriate (because
> we're checking if the target exists).

Sure, but I don't see any other places where we can use Object.assign right away, without any refactoring :)

Hey Charley, nice job! To land the patch please do the following:

* Squash your commits into one;
* Append "r=azasypkin, julien" to the commit message;
* Update your PR with this squashed commit and wait until all test pass(try-server-hook should automatically add comment with link to the test results once you update PR);
* Add "checkin-needed" to the bug "Keywords:" section.

Thanks!

Comment 9

[cvfergus]

Oleg,

Thanks for the directions, they were very helpful. I've done all of those things and "try-server-hook" posted a comment on the pull request with a "Results" link. Clicking on that link shows that there were no tests run which seems odd.

Charley

Comment 10

[Oleg Zasypkin [:azasypkin]]

(In reply to cvfergus from comment #9)
> Oleg,
> 
> Thanks for the directions, they were very helpful. I've done all of those
> things and "try-server-hook" posted a comment on the pull request with a
> "Results" link. Clicking on that link shows that there were no tests run
> which seems odd.
> 
> Charley

No worries, you just need to wait a bit for them to appear :)

Comment 11

[Oleg Zasypkin [:azasypkin]]

Removing checkin-needed for now, let's add it back once tests pass

Comment 12

[cvfergus]

Tests completed with 1 failure that I suspect (hope) is unrelated to the work I did. What should I do from here?

Comment 13

[Julien Wajsberg [:julienw]]

Yep, it's unrelated.

Thanks !

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/47f570e11a26d75f37c6f86f703d66fdbc603275