WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#37867 closed enhancement (fixed)

Document return types for create/create_and_get/create_many() methods of factory classes

Reported by: jdgrimes Owned by: DrewAPicture
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.7
Component: Build/Test Tools Keywords: has-patch
Focuses: docs Cc:

Description

The create(), create_and_get(), and create_many() methods are defined on the base factory class, and use other methods that are defined as abstract and implemented by the child classes. So just documenting the return types of the child class methods isn't really that helpful, and probably won't be picked up by IDEs. But by using @method annotations in the class-level docblocks for each of the factories we can provide this information in a form that will be helpful to those of use who use intelligent IDEs, as well as any developer who is new to the tests.

Attachments (1)

37867.diff (5.6 KB) - added by jdgrimes 4 years ago.

Download all attachments as: .zip

Change History (13)

@jdgrimes
4 years ago

#1 follow-up: @johnbillion
4 years ago

  • Keywords reporter-feedback 2nd-opinion added

Which IDE are you using? This sounds like something that an IDE should be able to handle easily without needing to be prompted by a potentially very long list of @method comments.

#2 @jdgrimes
4 years ago

  • Keywords reporter-feedback 2nd-opinion removed

37867.diff is a first pass, probably we should decide on a standard for the class descriptions here. What I used differs from the one for bookmarks, which is the only one that had a docblock. I have omitted @since tags, although they could also be added, and I guess would be 3.7.

#3 in reply to: ↑ 1 @jdgrimes
4 years ago

Replying to johnbillion:

Which IDE are you using? This sounds like something that an IDE should be able to handle easily without needing to be prompted by a potentially very long list of @method comments.

PhpStorm. It is only 3 method comments. I hope the patch makes it more clear, maybe my explanation wasn't very good.

#4 @jdgrimes
4 years ago

  • Keywords reporter-feedback 2nd-opinion added

Oops.

#5 @jdgrimes
4 years ago

  • Keywords reporter-feedback removed

#6 follow-up: @boonebgorges
4 years ago

The PHPDocumentation description for @method suggests that it's specifically for use in contexts where __call() is used on a parent class, so that there are no methods where the documentation could naturally appear. This isn't the case for create() and friends. I'm not sure how PhpStorm and similar IDEs handle this sort of thing, but it seems like they (or their users) should be able to traverse the inheritance tree for this sort of thing. https://www.phpdoc.org/docs/latest/references/phpdoc/tags/method.html

Now, it so happens that the classes in question - create_object(), update_object(), create(), etc on WP_UnitTest_Factory_For_Thing - do not have any PHPDoc at all. I bet proper documentation of these would be enough to make your IDE document the child classes properly.

Last edited 4 years ago by boonebgorges (previous) (diff)

#7 in reply to: ↑ 6 @jdgrimes
4 years ago

Replying to boonebgorges:

The PHPDocumentation description for @method suggests that it's specifically for use in contexts where __call() is used on a parent class, so that there are no methods where the documentation could naturally appear. This isn't the case for create() and friends. I'm not sure how PhpStorm and similar IDEs handle this sort of thing, but it seems like they (or their users) should be able to traverse the inheritance tree for this sort of thing. https://www.phpdoc.org/docs/latest/references/phpdoc/tags/method.html

Now, it so happens that the classes in question - create_object(), update_object(), create(), etc on WP_UnitTest_Factory_For_Thing - do not have any PHPDoc at all. I bet proper documentation of these would be enough to make your IDE document the child classes properly.

Well, the problem isn't that the methods themselves aren't documented, but that their return values aren't. Documenting them on the base class would require us to use generic values, which wouldn't help. By using the @method annotation, we're able to specify that calling $factory->user->create_and_get() will return a WP_User object, whereas $factory->site->create_and_get() will return a WP_Site. This is helpful fore code autocompletion, and just for documentation purposes in general.

It is true of course that advanced IDEs should theoretically be able to figure out this information, but that doesn't help devs who are new to the factories to understand exactly what is going on. The @method annotations provide explicit documentation that is helpful for everyone, I think.

#8 follow-up: @boonebgorges
4 years ago

@DrewAPicture Can you have a think about what @jdgrimes is proposing?

#9 in reply to: ↑ 8 @DrewAPicture
4 years ago

Replying to boonebgorges:

@DrewAPicture Can you have a think about what @jdgrimes is proposing?

Using @method annotations would definitely be the most efficient way to go, albeit copping out of doing it the "right" way, which would of course involve redefining and documenting all of the relevant superclass methods in each of the factories.

Using @method annotations in this way is a bit of a bastardization of their intent to be sure, but it gets the job done. I personally think in the specific context of our unit test factories, the level of potential confusion (in expecting magic methods) would be fairly low.

FWIW, we just implemented custom test factories in a plugin I work on and this is actually the same method we used to indicate return types.

#10 @DrewAPicture
3 years ago

  • Keywords has-patch added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.9

#11 @DrewAPicture
3 years ago

  • Owner set to DrewAPicture
  • Resolution set to fixed
  • Status changed from new to closed

In 40968:

Tests: Add @method notations to factor class DocBlocks as a way to indicate expected return types from factory methods for the benefit of IDEs.

Props jdgrimes.
Fixes #37867.

#12 @DrewAPicture
3 years ago

s/factor/factory :eye-roll:

Note: See TracTickets for help on using tickets.