Closed Bug 126337 Opened 23 years ago Closed 14 years ago

Super-review doc provides no guidance on adequate comments

Categories

(mozilla.org :: Governance, task, P5)

Product:
mozilla.org
Component:
Governance
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: choess, Unassigned)

References

(
URL
)

Details

The super-review page at <URL:http://www.mozilla.org/hacking/reviewers.html> and
the Code Reviewers guide at
<URL:http://www.mozilla.org/projects/seamonkey/rules/code_review.html> only
mention commenting of code in a very limited sense  (Tip 16 at the first URL). 
Because of this, a great deal of the comments within the Mozilla codebase to
explain interfaces, functions, &etc. are either outdated or non-existent.  (See
bug 125654.)

I would propose that we add to the guidelines for reviewers additional tips,
along these lines:

a) Unless a function or interface is highly trivial and its purpose apparent,
there should be a comment explaining its purpose.

b) Whenever code is changed so that a comment is no longer accurate, that
comment should be updated as part of the checkin of that code.

Comments should be in Javadoc format, so unreal.elemental.com can make sense of
them.

Thoughts?  Comments?  Criticism?  I think our rules on commenting should be
flexible, so we don't end up with a ridiculously formal scheme, but I also feel
that our current level of obsolete and missing comments is unacceptable and is
hindering increased outside involvement in the project.
cc'ing brendan who is owner of the super reviewer's guide
Here's a potentially good rule: *any* checkin that changes the signature of a
public interface method, or adds a public interface method, *must* have a
comment on that method.  Same with addition or significant changes to interface
classes.

This is waaaay more lacking than it should be in Mozilla.  I sort of understand
people being lazy about implementation class documentation, but interfaces? 
That's unacceptable.
I think the rules proposed by both jkeiser and choess sound quite reasonable,
and support them both.
Compare bug 125654.

I'd suggest that interfaces should (=rule-of-thumb) consist of *at least* 2-3
lines. Most of the time, you can think of really useful information to provide,
if you try to.
accepting QA for mozilla developer docs.

some of these bugs have been around for a _long_ time. Reporters, would you
please review the bugs, see if the issues have been resolved, and close bugs
appropriately.

I will do a full review of all bugs not touched in one week (8th April). 

Thanks.

</spam>
QA Contact: endico → imajes
I absolutely agree with all comments to this bug. Brendan?
Assignee: endico → zak
Component: Mozilla Developer → Governance
Product: Documentation → mozilla.org
QA Contact: imajes → governance
Version: unspecified → other
Is this still an issue - given that the bug is ~ 5 years old, I suspect that it hasn't been a burning issue.
Status: NEW → ASSIGNED
Zak, it is a burning issue, has always been. It has just been ignored by most developers. MDC has helped a great deal recently. But a lot of interfaces are still properly well documented. Esp. the interfaces itself (not the methods) are usually completely lacking comments.
It's not a burning issue, or we'd smell more smoke and feel flames. We don't, that includes from Ben (until comment 9), so please: don't exaggerate.

Specific interfaces that lack documentation should be the subject of new bugs.

Anyone who needs a one-size-fits-all doc, or a more nuanced doc, on how to comment interfaces, may not be ready for writing said interfaces. We need less "meta-doc" here and more plain old hard-sweaty-work doc-doc. IMHO,

/be
Brendan, we discussed this 7 years ago. I did feel flames back then, when I was trying to get my head around Mozilla. And you saw my smokes :). You said "We have no time, we'll do it for Mozilla 1.0". That long passed. Comments & docs have gotten better, but are still far below the reasonable level. 'Reasonable' being where it costs the developer far less time to add a comment than it costs others (combined) to gather the same information.

I'm not asking for a "meta-doc" here, this bug *specifically* requests that super-reviewers mandate that interfaces have a proper JavaDoc comment. That means maybe 4 lines of non-redundant explanation what the interface is about at all. Very simple, costs the developer 3 minutes. And it will be far more accurate than what other people assume and clear up misunderstandings, apart from simply saving huge amounts of time.

And don't say it's not needed. Even something as "obvious" as nsIDownloader is not. There's a completely unrelated nsIDownload, too, and one of them does upload as well (!), and the nsIDownloader implementation is mostly about temporary files, the actual download can be trivially done in JS without it. This is just one example, these cases are very common across the source. Thus, it makes no sense to file specific bugs, because I'd have to file bugs about 50% of all IDL files.

Regards,

Ben
If the amount of work required for annotating an interface is roughly as much as filing and managing a bug report, why don't we instead spend the effort trying to organize an "undocumented interface hunt, fix and release-fest"?

This could be a good project for a few students or interns or larval hackers who want to get a better feel for the code.
It should be somebody who knows the interface well and what the impl does, not a newbie who randomly adds stuff across the source. The information provided must be accurate.
Could a novice help by studying and documenting the simpler interfaces, with a review by more experienced devs after?

Perhaps a list of undocumented interfaces (which should be easy to programmatically generate) would help?

Work with me here. :) How can we solve or migitate this problem?
> How can we solve or migitate this problem?

See initial description.
I understand that a policy will be helpful. However, the policy alone won't fix the current problem, will it? I have worked on other projects that have policies requiring documentation and it hasn't helped much. Organizing teams of people to go in and fix things is what did the most good.
(In reply to comment #11)
> Brendan, we discussed this 7 years ago. I did feel flames back then, when I was
> trying to get my head around Mozilla. And you saw my smokes :). You said "We
> have no time, we'll do it for Mozilla 1.0". That long passed.

Yes, and now there's no smoke or fire. My point.

(In reply to comment #15)
> > How can we solve or migitate this problem?
> 
> See initial description.

That just says when and how to comment. Zak's trying to get to *who*, without which this is another boil-the-oceans junk bug.

/be
The who is also clear from initial proposal, even more so from comment 3.
I think that's a really good idea - if you change a method signature, you know the function very well. If you're then forced to add a comment, it takes you no effort, and it will be correct.

The same should happen with whole interfaces. (Whenever you change any function of it.) Just a few lines, non-redundant. That's all it takes.
So most interfaces that lack comments won't be touched, since most are frozen by default if not de-jure. So most won't ever gain comments. So forgive me for asking how adding a rule at this late date will help?

/be
(In reply to comment #17)
> (In reply to comment #11)
> > Brendan, we discussed this 7 years ago. I did feel flames back then,
> > when I was trying to get my head around Mozilla. And you saw my smokes :).
> > You said "We have no time, we'll do it for Mozilla 1.0". That long passed.
> 
> Yes, and now there's no smoke or fire. My point.

Well, for the OS/2 port (and I believe for BeOS, too) there are smoke and fire. And partly that is because the code in Thebes is undocumented. The interface methods have nice descriptions for the most part but almost all of the platform specific code that one needs to adapt for a port are completely undocumented.
I'm all for good code documentation, but IMO, the documentation shouldn't start with first principles for a particular topic.  The Thebes code should be comprehensible if someone has an understanding of general 2d gfx and the general way things get drawn in mozilla.  The generic interfaces have comments (though as I look, some of the comments are a little outdated, but not incomprehensible), and those are the interfaces that the platform specific code is built on top of -- you should be able to ignore all the existing platform specific code and just implement a new subclass of gfxPlatform or gfxASurface and have things work fine.  The font code is a little more opaque because it's still being heavily worked on, but the same principles should apply.

In either case, the comments there won't tell you how to implement a new gfx back end; that's for a separate document that someone might or might not write.
Vlad, that's why I wrote "partly". The other part is obviously due to my incompetence. ;-) I would just have been a lot easier if every function had at least a short comment at the top saying what it is trying to do. That can also be done for code that is still being worked on.
Thanks for the additional comments all!

This sounds like several different issues that are all bundled together:

* the super-review/code reviewers document should be reviewed (and likely updated)

* the super-review/code reviewers document should contain guidelines on code commenting and documentation

* our dev tools (including our culture) need to integrate commenting

* we need to work towards a more well-commented code-base

I will do some research on how other project have (or have not) solved similar issues and then post some broad proposals.

Cheers!
--zak
Triaging this bug with Frank and Gerv.

I'll file bugs for the individual issues raised in this bug.
Priority: -- → P5
CCing mconnor, who is currently revising this page.

Gerv
Assignee: zak → nobody
Status: ASSIGNED → NEW
Depends on: 501646
We have, in fact, removed these tips from that page entirely. The right place for such advice in 2010 is somewhere on MDC.

Gerv
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.