Page MenuHomePhabricator

Should function without parameter but with return statements require documentation with codesniffer?
Closed, ResolvedPublic

Description

The codesniffer sniff FunctionCommentSniff emits warnings (MissingDocumentationPublic, MissingDocumentationProtected, MissingDocumentationPrivate) when a function has parameters or is a "getter". But what about a function without params that returns a value but is not named get*?

Should this function require documentation of the return value?

If T258925: Do not warn about missing function doc comment when parameter and return types are fully specified by type hints get fixed a possible return type hint should not require documentation.

Event Timeline

Change #1133217 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/tools/codesniffer@master] Require documentation for parameterless function with return value

https://gerrit.wikimedia.org/r/1133217

To see the impact on wmf deployed code see patch sets list with this search https://gerrit.wikimedia.org/r/q/topic:docs+message:T272803

Change #1133217 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Require documentation for parameterless function with return value

https://gerrit.wikimedia.org/r/1133217

Lots of errors in the Translate extension now. How is one supposed to have comments like this now: https://gerrit.wikimedia.org/g/mediawiki/extensions/Translate/+/9d172de7d8a9fc802e403ddcce8a6b83050b415d/src/WebService/TranslationWebService.php#266? I could not figure any way to do it other than disabling the lint. The method itself has full type declarations so it doesn't need any function doc.

Are you sure this change caused the errors? This task is about

  • methods with a return statement (TranslationWebService::setLogger has no return statement),
  • which miss documentation (full type declaration should count as documentation).

I am not sure, but I think it is a recent change and I found this and T375033: Relax MediaWiki.Commenting.PropertyDocumentation.WrongStyle on typed properties and this looked like more likely cause.

Another recent change is rETRA48d9d2620c8ece6be7aa93ef2afcb600ab18aefa in the Translate repo, which is a much more plausible cause. (Actually, I don’t think the change described in the current task has been released yet, so it cannot affect Translate at all.)

(Actually, I don’t think the change described in the current task has been released yet, so it cannot affect Translate at all.)

Indeed, this task despite being Resolved isn't released yet (and given the normal sequencing of MW-CS versions, probably won't be for a few months).

Lots of errors in the Translate extension now. How is one supposed to have comments like this now: https://gerrit.wikimedia.org/g/mediawiki/extensions/Translate/+/9d172de7d8a9fc802e403ddcce8a6b83050b415d/src/WebService/TranslationWebService.php#266? I could not figure any way to do it other than disabling the lint. The method itself has full type declarations so it doesn't need any function doc.

Codesniffer (and maybe the code style) does not accept "grouping coments" like this or comments to allow folding of sections, when the following function does not have it's own comment.

With 48d9d2620c8ece6be7aa93ef2afcb600ab18aefa the exclude of the relevant sniffs were removed.
If you still need this code style the severity of the sniffs should be set to 0, to indicate this is a wanted behaviour.

.phpcs.xml
	<!-- Optional explain why -->
	<rule ref="MediaWiki.Commenting.FunctionComment.WrongStyle">
		<severity>0</severity>
	</rule>
	<!-- Optional explain why -->
	<rule ref="MediaWiki.Commenting.PropertyDocumentation.WrongStyle">
		<severity>0</severity>
	</rule>

(and given the normal sequencing of MW-CS versions, probably won't be for a few months).

That’s actually sad. I wasn’t a fan of the trainsperiment week (I think it sacrificed too much testing opportunity, which could have had bad consequences), but waiting months to see one’s code in production is the other extreme.

The codesniffer is a tool for developer, it not running in wmf production. There is T266890 to discuss the release process.

Other libraries are released more often and updated in core. But bringing a new codesniffer version into "production" needs to commit to ca. 850 git repos (https://libup.wmcloud.org/library/composer/mediawiki/mediawiki-codesniffer?branch=main), consuming time and resources. There is no perfect way for it.

Change #1184919 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/tools/codesniffer@master] HISTORY: Tag as v48.0.0

https://gerrit.wikimedia.org/r/1184919

Change #1184919 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] HISTORY: Tag as v48.0.0

https://gerrit.wikimedia.org/r/1184919