User "NeilK" posted a comment on MediaWiki.r95207. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/95207#c21315 Commit summary:
Add doxygen documentation to most functions in ArchiveLinks extension. Comment: @global isn't a recognized doxygen command, although I see that some other people in our community do use it. I don't think it helps because you're not communicating anything that the global keyword does. If you want to be helpful, indicate the file where the global variable is set. (However even that gets redundant with MediaWiki since most global things come from DefaultSettings.php or GlobalFunctions.php.) You don't need to say "this function"; that's clear. There's no reason to have a complete sentence, either. You can tell your English professor to call me. Try to make your comments more about what it's doing, and less about how you do it. The comment for delete_dups() is just about right. The one for parse_wget_log() has a few problems. * "Uses regular expressions"... we don't care... we're trying to scan the file to find out how to fix something. "Uses regular expressions" is no help, unless there's some central regular expression processing thing for the whole system. * "This is then returned to call_wget" ... I don't see any call_wget in that function! Don't document anything other than parameters and return values. You may want to move things around. And if you change the call_wget function name, you don't want to go looking for other functions to fix documentation. Keep docs *close* to the functionality. So, here's another way to write it: /** * Reads wget log to write file names and download statuses into $this->downloaded_files. * Also returns value of $this->downloaded_files. * Important: wget must log in non-verbose mode. * * @param $log_path string Path to the wget log file * @param $url URL of the page that was archived * @return array of strings, paths to downloaded files */ Incidentally, I didn't notice this before, but it's a bit funny that it both returns a value and sets a property on an object. Normally you wouldn't want to do both. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
