Closed Bug 1157377 Opened 7 years ago Closed 7 years ago

[mozdevice] Add parameter types to all adb*.py docstrings

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(firefox40 affected, firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- affected
firefox43 --- fixed

People

(Reporter: davehunt, Assigned: rsconceptx, Mentored)

References

Details

(Whiteboard: [lang=py][good first bug])

Attachments

(1 file, 3 obsolete files)

The adb*.py docstrings either fail to include the parameter types or mention them in the parameter descriptions. According to http://sphinx-doc.org/domains.html#info-field-lists the parameter types can be specified in-line (if the type is a single word) or separately.

For example:

    :param cmds: list containing the command and its arguments to be executed.

Would become:

    :param list cmds: the command and its arguments to be executed.

The code to be changed lives here: http://hg.mozilla.org/mozilla-central/file/default/testing/mozbase/mozdevice/mozdevice and should be limited to the files matching adb*.py
Hi, Dave, i would like to fix the bug. I am a newbie, this is my first time in bugzilla, can you guide me ?
The files you are referring to are the two files named "adb.py" and "adb_android.py" ?
If so, then should i download the raw versions of two files ?
Flags: needinfo?(dave.hunt)
(In reply to rsconceptx from comment #2)
> The files you are referring to are the two files named "adb.py" and
> "adb_android.py" ?
> If so, then should i download the raw versions of two files ?

That's correct. I didn't name them explicitly due to a patch from another bug that adds adb_b2g.py, but you don't need to worry about that. You will need to clone the mozilla-central repository, make the changes locally, and generate a patch for review.

Here are some guides that should help you:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Flags: needinfo?(dave.hunt)
Assignee: nobody → rsconceptx
Hi,Dave i downloaded the mozilla-central bundle,since i was unable to "hg clone" it. But now when i "hg unbundle" the downloaded mozilla-central bundle, it gives this error :

"abort: integrity check failed on data/content/media/test/VID_0001.ogg.i:cbdcb3ff4404!"

Is it possible for me to unbundle the repository beyond this error, or i have to download the bundle again ?
I'm not familiar with hg bundles. Did you follow the instructions here? https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial/Bundles. An integrity check sounds pretty fatal, so if you can't get it working it may be worth attempting to download the bundle again.
Is this issue still open...
(In reply to faizan10114 from comment #6)
> Is this issue still open...

Yes, this bug is open and assigned.
Status: NEW → ASSIGNED
Hi,Dave thanks for those guides, I now have a local copy of the "mozilla-central" repo. Ill update you when i am done making changes.
Hi,Dave,sorry for the late reply, i had my exams. I have modified the "adb_android.py" file for now. So can you give me the sequence of steps or commands to take to upload the patch of that file, for review, and if it its correct, ill upload the patch for the other file also. I have made a local clone of the main "mozilla-central" repo as "mozilla-central-docedit" and inside that i have made changes.

Thanks.
(In reply to rsconceptx from comment #9)
> Hi,Dave,sorry for the late reply, i had my exams. I have modified the
> "adb_android.py" file for now. So can you give me the sequence of steps or
> commands to take to upload the patch of that file, for review, and if it its
> correct, ill upload the patch for the other file also. I have made a local
> clone of the main "mozilla-central" repo as "mozilla-central-docedit" and
> inside that i have made changes.

The following guide will help you to generate a patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attached patch Added parameter types (obsolete) — Splinter Review
Hi Dave, here is the first set of patch i made for adb_android.py. Please review.
rsconceptx, you need to click the review link on the attachment and request the review by setting the select input to ? and add dave.hunt@gmail.com to the text input.
Flags: needinfo?(rsconceptx)
Comment on attachment 8639824 [details] [diff] [review]
Added parameter types

Review of attachment 8639824 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Rahul, just a few nits to address.

::: testing/mozbase/mozdevice/mozdevice/adb_android.py
@@ +63,5 @@
>              throwing an ADBTimeoutError.
>              This timeout is per adb call. The total time spent
>              may exceed this value. If it is not specified, the value
>              set in the ADB constructor is used.
> +        :type timeout: integer or None    

Nit: Please take care to avoid trailing whitespace.

@@ +186,1 @@
>              checked.

Let's move this up to the line above now that we've reduced the length.

@@ +216,1 @@
>          :param extras: Dictionary of extra arguments for application.

Nit: There's no need to mention the type in the description any more.

@@ +324,5 @@
>              This timeout is per adb call. The total time spent
>              may exceed this value. If it is not specified, the value
>              set in the ADB constructor is used.
> +        :type timeout: integer or None
> +        :param bool root: boolean flag indicating whether device is rooted or not.

Nit: Remove mention of type in description.

@@ +358,2 @@
>              uninstalled.
> +        :param bool reboot: boolean flag indicating that the device should

Nit: Remove mention of type in description.
Attachment #8639824 - Flags: review-
Flags: needinfo?(rsconceptx)
Hi, Dave uploaded the revised patch for "adb_android.py". Please review. Will upload the other patch of file "adb.py" once you approve this.
Thanks.
Attachment #8639824 - Attachment is obsolete: true
Attachment #8640505 - Flags: review?(dave.hunt)
Comment on attachment 8640505 [details] [diff] [review]
Fixed parameter types in adb_android.py

Review of attachment 8640505 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I just had one comment about the documentation of the root parameter of the stop_application method. Once this is addressed, I'll be happy to get this landed.

::: testing/mozbase/mozdevice/mozdevice/adb_android.py
@@ +322,5 @@
>              This timeout is per adb call. The total time spent
>              may exceed this value. If it is not specified, the value
>              set in the ADB constructor is used.
> +        :type timeout: integer or None
> +        :param bool root: Flag indicating whether device is rooted or not.

This is not quite correct. The flag indicates if the commands should be executed as root. See http://hg.mozilla.org/mozilla-central/file/32712cd01159/testing/mozbase/mozdevice/mozdevice/adb.py#l925
Attachment #8640505 - Flags: review?(dave.hunt) → review-
Hi, I have corrected that "root" boolean flag docstring in "adb_android.py", and I have also made changes to "adb.py", and attached a combined patch for both of the files. Please review.
Attachment #8640505 - Attachment is obsolete: true
Attachment #8642874 - Flags: review?(dave.hunt)
Comment on attachment 8642874 [details] [diff] [review]
Fixed params in adb.py and adb_android.py

Review of attachment 8642874 [details] [diff] [review]:
-----------------------------------------------------------------

With the single nit addressed this will be ready to land. Please also append r=dhunt to your commit message to indicate that I am the reviewer for this patch.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1702,5 @@
>              be killed. Note that only the first 75 characters of the
>              process name are significant.
>          :param sig: optional signal to be sent to the process.
> +        :type sig: integer or None
> +        :param integer attempts: number of attempts to try to 

Nit: Trailing whitespace
Attachment #8642874 - Flags: review?(dave.hunt) → review-
Hi,Dave
I have removed the trailing whitespace, as mentioned by you.
Please Review.
Attachment #8642874 - Attachment is obsolete: true
Attachment #8645349 - Flags: review?(dave.hunt)
Comment on attachment 8645349 [details] [diff] [review]
Fixed parameter types to adb.py and adb_android.py

Review of attachment 8645349 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!
Attachment #8645349 - Flags: review?(dave.hunt) → review+
I think we're safe without a try run here as the changes are only to docstrings.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/32d2aad9ca43
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.