Hi,
A great initiative, thank you!
I am generally in favor of this proposal but just want to give a
cautionary tale. It's a bit off-topic but important.
Given that there is no actual enforcing mechanism for the
documentation typehints, some of them have actually drifted from
reality. I caused a UBN bug once by relying on the documentation for
the type of a variable. So my request is to avoid mass migration of
documentation type hints to php type declaration.
Best
Am So., 30. Okt. 2022 um 14:02 Uhr schrieb Daniel Kinzler
<dkinz...@wikimedia.org>:
Thank you for suggesting this!
I agree that type declaration is preferable to type documentation,
and that type documentation is often redundant if type declaration
is present.
However, we can't always use type declarations. For instance,
union types are quite useful, since PHP doesn't have method
overloading. And union type declarations will only become
available in PHP 8. So we'll have a mix of declared and
un-declared parameters and fields for a while. I think we should
still require type documentation if there is no type declaration -
and of course, if a method has any @param tags, it needs to have
all of them.
Also there is the notable exception of the array type. Saying that
something is an array is generally insufficient, we should say at
least whether it's a list or an associative array, and document
the type of the array elements or and/or well-known keys.
And we should be careful that we don't end up discouraging
documentation of the meaning of a parameter. The barrier to adding
some explanation of the meaning of a parameter is lower if there
is already a @param string $name line. If I'd first have to create
a doc block, I may just not add the documentation at all. We
should still encourage having doc blocks in all but the most
trivial cases (simple constructors, getters and setters probably
don't need one).
-- daniel
PS: I'm not sure I like constructor argument property promotion...
For very simple value objects that might be nice, but generally, I
fear it will make it harder to see all fields declared on an object.
Am 28.10.2022 um 16:03 schrieb Lucas Werkmeister:
Hi all!
In my opinion, MediaWiki’s PHPCS ruleset feels largely rooted in
an older version of PHP, where static type declarations (formerly
known as “type hints”) did not exist. As we move towards more
modern code, I think some rules should be relaxed, and others
adjusted. More specifically, I’d like to know if most people
agree with the following propositions and conclusion:
Proposition 1: */Some/ code is sufficiently documented by names
and types*, and does not require additional documentation. Cases
where additional documentation is required do certainly exist,
but they can only be identified by human reviewers, not by
automated tools.
You can see this in our existing code wherever a doc comment
specifies only a type (with @var, @param, or @return), but no
additional text. For example, in CreditsAction
<https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/actions/CreditsAction.php>,
nobody needs to be told that the LinkRenderer will be used to
render links, or that the UserFactory creates User objects:
class CreditsAction extends FormlessAction {
/** @var LinkRenderer */
private $linkRenderer;
/** @var UserFactory */
private $userFactory;
Likewise, it’s not necessary to explain in great detail that the
string returned by LinksTable::getTableName()
<https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/deferred/LinksUpdate/LinksTable.php#175>
is the table name, that the $actor parameter of
ActorCache::remove( UserIdentity $actor )
<https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/user/ActorCache.php#103>
represents the actor to remove from the cache, or what the
meaning of the Message $m and returned MessageValue are in
Message\Converter::convertMessage()
<https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/Message/Converter.php#48>:
/**
* Convert a Message to a MessageValue
* @param Message $m
* @return MessageValue
*/
public function convertMessage( Message $m ) {
(I want to clarify that in this last example I’m only talking
about the @param and @return tags that already don’t have any
prose text. While the method comment “Convert a Message to a
MessageValue” might also be considered redundant, I think this
would be more contentious, and I’m not advocating for removing
that today.)
Proposition 2: *Adding types as static types is generally
preferable.* Unlike doc comments, static types are checked at
runtime and thus guaranteed to be correct (as long as the code
runs at all); the small runtime cost should be partially offset
by performance improvements in newer PHP versions, and otherwise
considered to be worth it. New code should generally include
static types where possible, and existing code may have static
types added as part of other work on it. I believe this describes
our current development practice as MediaWiki developers.
Note that some older MediaWiki classes are considered unsuitable
for static types, and should only be used in comments; this is
expected to help in a future migration away from these classes
(see T240307#6191788
<https://phabricator.wikimedia.org/T240307#6191788>).
Proposition 3: *Where types can be losslessly represented as PHP
static types, types in doc comments are unnecessary.* When doc
comments are considered necessary for actual documentation beyond
types, then the type is generally still included (and Phan will
check that it matches the static type), but when no further
documentation is needed (see proposition 1 above), then the @var,
@param, etc. doc comment can be omitted.
Note that depending on the PHP version, not all types can be
losslessly represented as PHP static types yet (e.g. union types
and mixed both need to wait for PHP 8.0, null and false for PHP
8.2); in such cases, doc comments can remain necessary.
Conclusion: *We should update our PHPCS ruleset to require fewer
doc comments.* Exact rules are probably to be decided, depending
on how much work we’re willing to put into the sniff
implementations (e.g. is it feasible to require /** @param */ doc
comments only if a parameter has no static type?), but generally,
I argue that we want code such as the following to be allowed by
our standard PHPCS ruleset:
class CreditsAction extends FormlessAction {
private LinkRenderer $linkRenderer;
private UserFactory $userFactory;
/** Convert a Message to a MessageValue */
public function convertMessage( Message $m ): MessageValue {
When doc comments are still necessary or at least beneficial
because the type alone isn’t enough information, it’s up to
humans to decide this while writing the code or point it out
during code review.
What do people think about this? :)
PS: In PHP 8, we could abbreviate some of this code even more
using constructor property promotion:
class CreditsAction extends FormlessAction {
public function __construct(
Page $page,
IContextSource $context,
private LinkRenderer $linkRenderer,
private UserFactory $userFactory
) {
parent::__construct( $page, $context );
}
(Again, I’m not saying that all code should look like this – but
I think we have plenty of existing code that effectively carries
no additional information in its documentation, and which could
be converted into this form without losing anything.)
Cheers,
Lucas
--
Lucas Werkmeister (he/er)
Software Engineer
Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin
Phone: +49 (0)30-577 11 62-0
https://wikimedia.de
Imagine a world in which every single human being can freely
share in the sum of all knowledge. Help us to achieve our vision!
https://spenden.wikimedia.de
Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens
e. V. Eingetragen im Vereinsregister des Amtsgerichts
Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig
anerkannt durch das Finanzamt für Körperschaften I Berlin,
Steuernummer 27/029/42207.
_______________________________________________
Wikitech-l mailing list --wikitech-l@lists.wikimedia.org
To unsubscribe send an email towikitech-l-le...@lists.wikimedia.org
https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
--
Daniel Kinzler
Principal Software Engineer, Platform Engineering
Wikimedia Foundation
_______________________________________________
Wikitech-l mailing list -- wikitech-l@lists.wikimedia.org
To unsubscribe send an email to wikitech-l-le...@lists.wikimedia.org
https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
--
Amir (he/him)
_______________________________________________
Wikitech-l mailing list --wikitech-l@lists.wikimedia.org
To unsubscribe send an email towikitech-l-le...@lists.wikimedia.org
https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
