Daniel Kinzler <dkinz...@wikimedia.org> writes:
> 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.
+1, there is also \stdClass which we use in a few places. I believe IDEs will
attempt to create placeholders for all
parameters as well, if you start documenting a single one.
> 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).
I agree with this as well.
Kosta
>
> – 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, 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() is the
> table name, that the $actor parameter of ActorCache::remove( UserIdentity
> $actor ) represents the actor to remove
> from the cache, or what the meaning of the Message $m and returned
> MessageValue are in
> Message\Converter::convertMessage():
>
> /**
>
> * 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).
>
> 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 to wikitech-l-le...@lists.wikimedia.org
> <https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/>
_______________________________________________
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/
