Opened 4 years ago

Closed 3 years ago

#16873 closed enhancement (fixed)

add javadoc to metrics-lib

Reported by: iwakeh Owned by: karsten
Priority: Low Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: #18746 Points:
Reviewer: Sponsor:

Description

Steps for providing javadoc for metrics-lib:

  • Change method comments to javadoc style, i.e. from /* ... */ to /** ... */
  • Add a javadoc ant-task that renders the documentation into a seperate folder docs.
  • Make sure the contents of the comments is still up-to-date.

These steps should be provided in three patches.
Test classes should not be included yet.

Child Tickets

Attachments (2)

ServerDescriptor.java (10.1 KB) - added by karsten 3 years ago.
Newly documented ServerDescriptor.java
suggestions.plain.diff (8.1 KB) - added by iwakeh 3 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 4 years ago by karsten

Severity: Normal

We should also consider producing a descriptor-$version-javadoc.jar.

Changed 3 years ago by karsten

Attachment: ServerDescriptor.java added

Newly documented ServerDescriptor.java

comment:2 Changed 3 years ago by karsten

Status: newneeds_review

Please find the newly documented ServerDescriptor.java attached to this ticket. What do you think, is this too verbose, still not detailed enough, too confusing in some other way, missing something important, etc.?

comment:3 Changed 3 years ago by iwakeh

This looks great!

I have some very minor additions:

For getSocksPort() near line 33:
Maybe, add that 0 is returned because SOCKSPort is deprecated. Otherwise, it might seem odd to someone unfamiliar with the descriptor format.

Maybe add a link to the respective extra info methods to getReadHistory() and getWriteHistory() near
line 124 and line 130?

And the copyright year could be increased to 2016?

comment:4 Changed 3 years ago by iwakeh

Parent ID: #18746

added parent:

The release should just include all javadoc that can be easily added.

All what's left could go into a new ticket "complete javadoc for metrics-lib".

comment:5 Changed 3 years ago by iwakeh

Last edited 3 years ago by iwakeh (previous) (diff)

comment:6 Changed 3 years ago by karsten

Status: needs_reviewneeds_information

There, I pushed my earlier patch to my javadoc branch and incorporated your feedback (thanks for that!).

However, I think we'll have to include things like @return in the comments, and that leads to a whole lot of duplicate text with our interfaces full of getters. And there are probably other conventions that we're violating in the current comments.

Do you happen to know some good guidelines for writing javadoc comments, ideally one that is commonly used by other Java projects as well? I would say let's take what Oracle has on their website, but maybe there are better guidelines.

comment:7 Changed 3 years ago by iwakeh

Thanks for the link to Oracle's javadoc style guide!
I'll add it to the list for discussion about the guide docs.

A longer comment below.
Maybe, for release 2.0.0 keep the javadoc comments as they are (here in ServerDescriptor)
and make additions once we have the guide docs in place?
Javadoc rules should definitely be part of the contributor's guide.

At the very end of this comment there is a diff with a javadoc ant task
for metrics-lib.

thoughts about comments

Well, some conventions are not that useful in my opinion.

It's not that helpful to read something like

/**
 * This method returns my foo.
 * 
 * @return Foo - the Foo
 */
 public Foo getFoo(){
    return myFoo;
}

or

/**
 * This method returns the bar belonging to foo.
 * @param foo the foo that determines the bar
 * @return the bar for foo 
 */
 public Bar retrieveBarOfFoo(Foo foo){
    ...
}

Such comments just clutter the java file uselessly, b/c the code
(or the methods' signatures) actually speaks well for itself.
I prefer reading the code to reading an outdated or
halfhazardly written comment that communicates nothing or
even something wrong.

It is quite a lot of work to keep comments as useful as they are
in the jdk javadoc, for example.

From a pragmatic point of view and for a project with a small
number of developers, I think the javadoc you added to ServerDescriptor
is currently the best approach.

This

/** Return the SHA-256 descriptor digest, encoded as 43 base64
  * characters without padding characters, that may be used to reference
  * this server descriptor from a network status descriptor. */
 public String getServerDescriptorDigestSha256();

I find easier to read than

/** Return the SHA-256 descriptor digest, encoded as 43 base64
 * characters without padding characters, that may be used to reference
 * this server descriptor from a network status descriptor.
 *
 * @return the String containing the SHA-256 digest */
public String getServerDescriptorDigestSha256();

simple Javadoc task for metrics-lib

(actually stolen from metrics-db :-)
There is no javadoc ant task yet in metrics-lib:

  • build.xml

    diff --git a/build.xml b/build.xml
    index f7ef7ca..9eb2c0b 100644
    a b  
    22  <property name="release.version" value="1.1.0-dev" />
    33  <property name="sources" value="src"/>
    44  <property name="classes" value="classes"/>
     5  <property name="docs" value="javadoc/"/>
    56  <property name="tests" value="test"/>
    67  <property name="libs" value="lib"/>
    78  <property name="jarfile" value="descriptor-${release.version}.jar" />
     
    2728      <fileset dir="${classes}" defaultexcludes="false" includes="**" />
    2829    </delete>
    2930    <delete file="${jarfile}"/>
     31    <delete file="${docs}"/>
    3032    <delete file="${jarsourcesfile}"/>
    3133    <delete file="${release.tarball}"/>
    3234  </target>
    3335
    3436  <target name="init">
    3537    <mkdir dir="${classes}"/>
     38    <mkdir dir="${docs}"/>
    3639  </target>
    3740
    3841  <target name="compile"
     
    5053    </javac>
    5154  </target>
    5255
     56  <target name="docs">
     57    <mkdir dir="${docs}"/>
     58    <javadoc destdir="${docs}">
     59      <classpath refid="classpath"/>
     60      <fileset dir="${sources}/" includes="**/*.java" />
     61    </javadoc>
     62  </target>
     63
    5364  <target name="test" depends="compile">
    5465    <javac destdir="${classes}"
    5566           srcdir="${tests}"

comment:8 Changed 3 years ago by iwakeh

The package descriptions for org.torproject.descriptor and org.torproject.descriptor.impl,
i.e., the two package-info.java files should be added. If possible for release 2.0.0.

comment:9 Changed 3 years ago by karsten

I tend to agree with you regarding @return comments. Though even Google's Java Style guide requires that "at-clause" to "never appear with an empty description." And I think we'd have to disable some warnings in Eclipse and Ant if we want to leave them out. Not saying that we must include them, but this seems like an important decision to make. I'm fine postponing it and including simple Javadoc comments in 2.0.0.

Oh, and let me make this discussion slightly more difficult: I pondered adding a DescriptorGenerator a while ago, where one would construct a new instance of a Descriptor, call its setters to give it some values, and then have the generator generate a text representation of that descriptor. This would be useful for testing and maybe other things. Now, would the Javadoc of these setters copy the Javadoc of the getters or simply link to them? Or would we use some smarter pattern for building these instances? (And is there a smarter pattern for parsed descriptors than a huge interface with dozens of getters?)

Regarding package descriptions, having one for org.torproject.descriptor would be very valuable. But my original plan was to exclude org.torproject.odescriptor.impl from Javadoc entirely. Is that crazy?

comment:10 Changed 3 years ago by karsten

Pushed a tweaked (I think) Ant task for building Javadocs to the javadoc branch.

comment:11 Changed 3 years ago by karsten

Pushed another commit with better docs for extra-info descriptors. That commit also contains class-level documentation for server and extra-info descriptors.

comment:12 in reply to:  10 Changed 3 years ago by iwakeh

Replying to karsten:

Pushed a tweaked (I think) Ant task for building Javadocs to the javadoc branch.

Thanks for correcting the clean task!

comment:13 in reply to:  9 Changed 3 years ago by iwakeh

Replying to karsten:

Oh, and let me make this discussion slightly more difficult: I pondered adding a DescriptorGenerator a while ago, where one would construct a new instance of a Descriptor, call its setters to give it some values, and then have the generator generate a text representation of that descriptor. This would be useful for testing and maybe other things. Now, would the Javadoc of these setters copy the Javadoc of the getters or simply link to them? Or would we use some smarter pattern for building these instances? (And is there a smarter pattern for parsed descriptors than a huge interface with dozens of getters?)

Interesting. Let's move this discussion to #18797.

Regarding package descriptions, having one for org.torproject.descriptor would be very valuable. But my original plan was to exclude org.torproject.odescriptor.impl from Javadoc entirely. Is that crazy?

Not at all crazy. I thought that the package javadoc for impl could simply state general information, like:

  • this is _the_ reference implementation of the descriptor api
  • design goals for the implementation
  • advantages as well as areas for improvement, if known
  • and that there is no other javadoc inside impl
  • and ...

comment:14 Changed 3 years ago by karsten

Okay, I agree that it makes sense to add package-level documentation for the implementation package for such general remarks. But do you know how to include that package-info.java in the generated Javadocs while excluding all other classes from that package? If there's no easy way, maybe we can make these general remarks in DescriptorSourceFactory where we might want to document how to use alternative implementations by setting system properties.

comment:15 Changed 3 years ago by karsten

I'm still working on this ticket, but it's a huge effort to write useful documentation for all interfaces and classes. Here's where I am, where a point is roughly 30 minutes of work:

File (in src/org/torproject/descriptor/) Remaining effort in points
BandwidthHistory.java 0
BridgeExtraInfoDescriptor.java 0
BridgeNetworkStatus.java 2
BridgePoolAssignment.java 1
BridgeServerDescriptor.java 0
Descriptor.java 2
DescriptorCollector.java 1
DescriptorDownloader.java 4
DescriptorFile.java 2
DescriptorParseException.java 1
DescriptorParser.java 2
DescriptorReader.java 4
DescriptorRequest.java 2
DescriptorSourceFactory.java 2
DirectoryKeyCertificate.java 2
DirectorySignature.java 2
DirSourceEntry.java 2
ExitList.java 1
ExitListEntry.java 1
ExtraInfoDescriptor.java 0
ImplementationNotAccessibleException.java 1
Microdescriptor.java 0
NetworkStatusEntry.java 4
package-info.java 0
RelayDirectory.java 2
RelayExtraInfoDescriptor.java 0
RelayNetworkStatus.java 4
RelayNetworkStatusConsensus.java 2
RelayNetworkStatusVote.java 4
RelayServerDescriptor.java 0
RouterStatusEntry.java 1
ServerDescriptor.java 0
TorperfResult.java 4
Total 53

If I prioritize this ticket over other tickets I can spend 4 hours = 8 points per work day on writing documentation. That means I'll be done around May 27.

comment:16 Changed 3 years ago by karsten

Status: needs_informationaccepted

Down by 25 points today, but the remaining 28 points might take longer. I'll also need to squash and split commits into bugfixes and documentation patches before merging. See my javadoc branch if you want, but don't review in detail just yet.

comment:17 Changed 3 years ago by karsten

Status: acceptedneeds_review

Good news: I'm even worse at estimating effort than I had thought. Down to 0 points by now!

Please review my branch task-16873 including the new code changes preceding the huge Javadoc commit. That last commit is really big, so maybe it's sufficient to look at package-info.java (curious to hear what you think about the goals and non-goals stated there) and then a random subset of other interfaces/classes.

Thanks in advance!

comment:18 Changed 3 years ago by iwakeh

Wow! That was very fast!

I still would like to read a little more of the javadoc.

When looking (w/o reading much yet) at the resulting HTML:

  • we could add the copyright notice as footer
  • the usage pages are often helpful when getting acquainted with a new api
  • window title and
  • overview page title are missing.

For these we need some more attributes in the javadoc task:

  <javadoc destdir="${docs}"
           footer="(C) 2016 The Tor Project, Inc."
           doctitle="metrics-lib API Documentation"
           use="true"
           windowtitle="metrics-lib API Documentation">
    <classpath refid="classpath"/>
    <fileset dir="${sources}">
      <exclude name="**/impl/**"/>
    </fileset>
  </javadoc>

It might be useful to introduce the @since <version> tags for all the documented methods?
(as the java jdk does, there I really need it often when switching between java versions I only need to keep the latest doc around.)

As said above; I'll get back to the javadoc content in another comment soon.

comment:19 Changed 3 years ago by iwakeh

I forgot:

All links need a full package reference to work, e.g.
{@link org.torproject.descriptor.ServerDescriptor}.
Currently, there are no links.

comment:20 in reply to:  19 Changed 3 years ago by iwakeh

Replying to iwakeh:

Currently, there are no links.

this refers to package-info.java, the other pages are fine.

comment:21 Changed 3 years ago by karsten

Great feedback! I made the changes you suggest above and pushed a few more commits to the same branch. Please keep sending more feedback!

comment:22 Changed 3 years ago by iwakeh

fast again :-)
DescripTor API and the real copyright logo look good!

A few things I noticed immediately:

  • the references to constants in DescriptorSourceFactory are lacking the #, e.g., {@link #COLLECTOR_DEFAULT}.
  • package-info is now fine except org.torproject.descriptor.impl should not be referenced as link, b/c it's excluded from javadoc, but this only avoids a warning when running javadoc
  • same with ExitList.java:83: warning - Tag @link: missing '#': "getEntries()"

still need to read the text ;-)

comment:23 Changed 3 years ago by karsten

Thanks! Please find the same branch with a new commit that fixes those issues.

comment:24 Changed 3 years ago by iwakeh

Good documentation! Quite a lot to read!

I put myself in the mindset of someone who reads the javadoc for the first time and wants to use descriptor quickly for accessing the available data.

Here some suggestions derived from that way of reading:

  • the overview page is missing (I added a diff, not a format patch just a suggestion, most of the text moved from the package description)
  • listing of the property names in the factory class and an example (also in the attached diff)
  • It might be very useful to add source code examples for using the descriptor api to the downloader, collector, parser, and reader classes. Maybe, just copied from Onionoo sources.
  • Somewhere there ought to be a link to the Tor spec. Maybe, in the overview or/and the classes?
  • the link in BridgeExtraInfoDescriptor should be turned into a clickable one.

Changed 3 years ago by iwakeh

Attachment: suggestions.plain.diff added

comment:25 in reply to:  24 Changed 3 years ago by karsten

Replying to iwakeh:

Good documentation! Quite a lot to read!

I put myself in the mindset of someone who reads the javadoc for the first time and wants to use descriptor quickly for accessing the available data.

Here some suggestions derived from that way of reading:

  • the overview page is missing (I added a diff, not a format patch just a suggestion, most of the text moved from the package description)

Looks good, applied with minor edits and pushed to my task-16873 branch.

  • listing of the property names in the factory class and an example (also in the attached diff)

Also applied.

  • It might be very useful to add source code examples for using the descriptor api to the downloader, collector, parser, and reader classes. Maybe, just copied from Onionoo sources.

Good idea, I included source code examples for collector and reader, as I expect those to be used by 90% of users. Parser is mostly an internal thing that happens to be publicly available, and downloader is not really used by anything and should be removed sooner than later.

  • Somewhere there ought to be a link to the Tor spec. Maybe, in the overview or/and the classes?

Sure, added to the overview.

  • the link in BridgeExtraInfoDescriptor should be turned into a clickable one.

Changed, as well as similar links in the other three Bridge* interfaces.

Great feedback, let me know if you have more, and I'll incorporate that. Thanks!

comment:26 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

I think is is well done and should be released.

comment:27 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Great! I just squashed commits and merged to master. Thanks again! Closing.

Note: See TracTickets for help on using tickets.