Closed Bug 1133076 Opened 5 years ago Closed 5 years ago

Add documentation for mozinstall

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: wlach, Assigned: wlach)

Details

Attachments

(1 file, 1 obsolete file)

We don't have documentation for mozinstall in mozbase. We should.
Attached patch Document mozinstall (obsolete) — Splinter Review
Attachment #8564359 - Flags: review?(ahalberstadt)
Comment on attachment 8564359 [details] [diff] [review]
Document mozinstall

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

Lgtm, consider the suggestions optional.

::: testing/mozbase/docs/mozinstall.rst
@@ +2,5 @@
> +====================================================================
> +
> +mozinstall is a small python module with several convenience methods
> +useful for installing and uninstalling a gecko-based application
> +(e.g. Firefox) on the desktop.

You could move this paragraph to the module docstring in __init__.py and add:
.. automodule:: mozinstall

here (without the :members: directive).

@@ +21,5 @@
> +API Documentation
> +-----------------
> +
> +.. automodule:: mozinstall
> +   :members: is_installer, install, get_binary, uninstall

I prefer not to explicitly list members since adding new members usually results in them being undocumented. You'd need to change the ..automodule directive to:
.. automodule:: mozinstall.mozinstall

(or use .. currentmodule or something like that).
Attachment #8564359 - Flags: review?(ahalberstadt) → review+
I think I'm going to more or less stick with the original approach. Comments below...

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> Comment on attachment 8564359 [details] [diff] [review]
> Document mozinstall
> 
> Review of attachment 8564359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lgtm, consider the suggestions optional.
> 
> ::: testing/mozbase/docs/mozinstall.rst
> @@ +2,5 @@
> > +====================================================================
> > +
> > +mozinstall is a small python module with several convenience methods
> > +useful for installing and uninstalling a gecko-based application
> > +(e.g. Firefox) on the desktop.
> 
> You could move this paragraph to the module docstring in __init__.py and add:
> .. automodule:: mozinstall
> 
> here (without the :members: directive).

I'm not sure if that's obviously better to be honest. I suspect the documentation is easier to modify if all the text is in an .rst file (and that seems to be what we mostly do elsewhere with mozbase docs)

> @@ +21,5 @@
> > +API Documentation
> > +-----------------
> > +
> > +.. automodule:: mozinstall
> > +   :members: is_installer, install, get_binary, uninstall
> 
> I prefer not to explicitly list members since adding new members usually
> results in them being undocumented. You'd need to change the ..automodule
> directive to:
> .. automodule:: mozinstall.mozinstall
> 
> (or use .. currentmodule or something like that).

Hmm, but if I do that everything gets confusingly prefixed with mozinstall.mozinstall

I couldn't figure out a way around this, and some quick googling indicates no one else could either. Let's just leave things as they are. I did find that we weren't documenting some exceptions though, I'll add them.
Attached patch Updated patchSplinter Review
Updating patch, carrying forward r+
Attachment #8564359 - Attachment is obsolete: true
Attachment #8565533 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d979b7a7ed9c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.