979661 - [marionette-js-client] The documentation for the scriptWith function is not correct.

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[Evan Tseng [:evanxd]]

It should say that return the result of script execution.
Please refer to https://github.com/mozilla-b2g/marionette-js-client/blob/master/lib/marionette/element.js#L100.

Comment 1

[AlexSue]

I want to know whether I need to understand about the executeScript method or not?
it is just a task I add the something lost to annotation?

Comment 2

[Evan Tseng [:evanxd]]

Hi 翌帆,

Sorry, late for the reply.
For this first bug, I don't need to know the detail of executeScript method.
You could just add `@return {String|Number} return value of the script.` at https://github.com/mozilla-b2g/marionette-js-client/blob/master/lib/marionette/element.js#L100.

Send a pull request with the change, and send your mentor a review request with following the https://speakerdeck.com/eragonj/all-about-firefox-os?slide=68 doc.

Thanks.

Comment 3

[AlexSue]

Attachment: add the annotation in scriptWith method

Comment 4

[(Inactive after June) George Duan [:gduan] [:喬智]]

Hi ,
please check your attachment again, you should paste the link of your pull request , like https://github.com/mozilla-b2g/gaia/pull/11111 .
Thanks.

Comment 5

[AlexSue]

Comment on attachment 8389697 [details]
add the annotation in scriptWith method

https://github.com/mozilla-b2g/marionette-js-client/pull/88

Comment 6

[AlexSue]

Comment on attachment 8389697 [details]
add the annotation in scriptWith method

https://github.com/mozilla-b2g/marionette-js-client/pull/88

Comment 7

[AlexSue]

Attachment: add the annotation in scriptWith method

Comment 8

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8389697 [details]
add the annotation in scriptWith method

remove it

Comment 9

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8389916 [details] [review]
add the annotation in scriptWith method

Hi,

I have put my comments on github, please modify it and set the review flags to me again.

Comment 10

[AlexSue]

Comment on attachment 8389916 [details] [review]
add the annotation in scriptWith method

I am Sorry that I don't use it well.
so there are many repeat comment or request.
I modified it. Thanks for your comment

Comment 11

[(Inactive after June) George Duan [:gduan] [:喬智]]

That's totally fine!

Just two more things, 

1. I don't think two separated commits is necessary. Could you combine them into one commit and push again?
2. In order to trace the bug easier from commit log in the future, I suggest to modify your commit message as the bug title "Bug 979661 - [marione........" , so that, we can use "git blame" to see which bug has done the modification.
( I saw your msg is "Update comment in line 100", it's not good, since the location will change eventually.)

we're almost there!

Comment 12

[(Inactive after June) George Duan [:gduan] [:喬智]]

btw, that'll be more convenient if you put your English name bugzilla ;)

Comment 13

[AlexSue]

Attachment: Update the Bug with your suggestion and send new pull request.

I pull new request and correct my mistake.
please review again and sorry about that I don't know how to combine two commit into one.

Comment 14

[(Inactive after June) George Duan [:gduan] [:喬智]]

Hi Alex,

In reply to comment 13,
for example, you have root commit, and your commit#1 and commit#2, you can rebase to root commit with |git rebase -i "rootcommit hash"| , or reset to root commit and commit again (git reset "root commit hash" ; git commit -m "xxx")


I noticed that the travis failed with timeout error. Although, it's absolute not related to your modification. I still suggest you to have the travis rerun and make it green.

Normally, we'll modify the commit message and push again, so that github would think the two commits are different.

All of the above are just processes, you'll get more familiar with that!

Comment 15

[AlexSue]

Attachment: Update the Bug with your suggestion and send new pull request.

I sent a new pull request.
Well,I want to ask about the combine two comment question.
I need to clone all the project then combined the comments.
Finally,pull it to the github right?

Comment 16

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8391029 [details] [review]
Update the Bug  with your suggestion and send new pull request.

Good job, set Evan to check it again.

Comment 17

[Evan Tseng [:evanxd]]

Hi Alex and George,

r+, nice shot.

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

Merge into master,
https://github.com/mozilla-b2g/marionette-js-client/commit/49bac73cb7270827cb2b92bff16aa18ab0348fc1