Javadoc, modding API, and fun with annotations

[Cervator]

So recently we beat down the remaining broken unit tests, fixed lots of warnings, and generally dealt the gremlins of code chaos a mighty blow! I think maybe it is time to turn up the dial up the amount of analytics - specifically javadoc or the lack thereof

I have a separate writeup on more automated testing, but that's a topic for another day, still working on it slowly together with a similar setup at work. But one warning today after merging #1431 stuck out as interesting, and I'm really glad we had something flagging it!

22:31:36.728 [main] WARN  o.t.l.console.internal.CommandInfo - Parameter 0 in method public java.lang.String org.terasology.logic.permission.PermissionCommands.usePermissionKey(java.lang.String,org.terasology.entitySystem.entity.EntityRef) does not have a CommandParam annotation

First of, kudos for putting that warning in, whomever did it We need more of that!

The fix was easy enough, and I understand what it is for (the in-game help system doesn't know what we name our parameters, so CommandParam tells it). But wanting to embark on a quest to have (probably) Checkstyle nag for missing Javadoc made me think about the lack of Javadoc on this otherwise documented public method in a public class.

@Command(shortDescription = "Use an one time key to get op permission",helpText = "The config file contains a one time key which can be used to get op permission",runOnServer = true, requiredPermission = "")
public String usePermissionKey(@CommandParam("key") String key, EntityRef client) {

The most obvious Javadoc setup is to nag for classes, packages, and public methods, excluding private (and maybe protected/default?). But this spot would get caught by that, yet it is documented.

Anybody got any useful Javadoc conventions or awareness of the topic to help us figure out what is the best approach to encourage good doc yet not nag overly much, especially when there is in fact a type of documentation?

I know, I know, code is supposed to be self-documenting, but IMHO it really helps with at least a single sentence for anything but the most obvious (setters and getters). One small sentence isn't too much to ask, I hope? Any neat new Java8 magic that can help vanquish boilerplate so we don't need to doc it? Any easy way to nag for a sentence instead of an inserted template + author tag?

On annotations in particular I found references to the "Documented" annotation, which indicates to include annotation info in javadoc or javadoc-alikes. Instead of adding Javadoc that'll overlap with annotations like above (you'd normally have a descriptor for the "key" parameter both as an annotation and as a parameter in the javadoc) you simply throw "@Documented" in with the remaining annotations. Anybody tried that before? I figure that might hold more potential than to somehow pull the parameter name from a Javadoc parameter entry instead of having an annotation? Although I've been wrong before!

If we could use something like that and get both worlds covered with one set of statements that would be sweet, and could help avoid false positives (... if Checkstyle can be configured to consider "@Documented" as doc, anyway)

To also help a third angle on the topic we have a moldy Modding API page in the GitHub wiki that isn't doing much since it is separate from the code. It really needs some love. How awesome would it be if we could auto-export javadoc/annotation details for everything specifically annotated with our "@API" (allowing modules to use something in the engine - although there's a new permission setup in gestalt-module, does that make it more complex?) to an official code-based Modding API we can publish separate from the javadoc, somewhere between javadoc and the "normal" documentation in the wiki? That could incidentally also become our official API definition as per SemVer

That's probably enough buzzwords and topics for one day, any thoughts on the topic? I'd like to enable Javadoc nagging soon'ish, but would prefer to cause the least amount of grumbling and the most possible usefulness with a good rule definition

When we sort it out we should post a more official thread in the main project forum. But I figured it could be a while before we have a clear picture and didn't want to muddy up the top posts in such a thread.

 

[Limeth]

Since I am new to Terasology, I would greatly appreciate improvements in the documentation - it's kind of hard to get around at the moment. I'm currently learning the basics from other modules on GitHub. Thanks for investing the time.

 

[Marcin Sciesinski]

I think the best requirement is to have interfaces documented. This provides documentation for the abstraction and people should start using interfaces a bit more than they probably already do.

 

[manu3d]

As I'm no expert in writing code overall I'm chipping in only because I do have a useful angle on the matter: that of the relatively inexperienced programmer. This project is quite tricky to get into from a code perspective. Assuming that this is not an intended strategy to keep out people with my level of expertise, I'm wondering if the "ideal" of self-documenting code is clashing with the more gritty reality that the codebase isn't, in-fact, quite as self-documenting as the ideal it strives for.

I mean, I don't think there is a lot of code that can be safely considered self-documenting unless multiple people have reviewed it for that specific purpose rather than for pure functionality. Of course the code -I- write is perfectly self-documenting. Except when a second person jump in it and, how strange, finds that he doesn't understand a few details, he'd do things differently here and there and so on. In this context, and assuming solid code-reviews will remain unlikely given the limited resources, I would suggest some redundancy would be a good thing. That is, yes, I will write self-documenting code, HOWEVER, it would be good if checkstyle nags me for interfaces, classes and public methods to be documented with non-template javadocs. And ALSO, it would be good if PRs are left on hold until some appropriate level of documentation is in place, at least for new files, ideally for all modified portions of files, even better for whole file even if modified in small part.

Also consider that lack of documentation is a self-perpetrating phenomenon: people jump in, see not much documentation, adopt the project's culture and when they leave they leave code that requires an unnecessary amount of efforts to understand, modify or even cut out, effectively worsening the problem. So, the later the issue is tackled the more undocumented code will pile up, compounding the problem and making it harder to turn the culture around.

To conclude, a couple of practical suggestions: 1) PRs should be evaluated not only for functionality but also for presence/lack of good documentation. This I realize slows down the process but, in the longer term, should make changes and addition easier, and should allow new members to start being productive earlier, probably increasing membership and retention. 2) I forgot. No, really. I had something else in mind but I forgot. Hopefully I'll remember tomorrow.

Good night!

 

[Cervator]

Definitely agreed on PRs and doc, which would help if Checkstyle nags over missing doc

We could strive to get into requiring more unit testing too, but I think the barrier of entry is higher there than just adding some javadoc. We really need some good guides on good unit testing specific to Terasology. But that's probably a different topic.

In short: More analytics on PRs + more conventions on what to review and expect = good!