bevy icon indicating copy to clipboard operation
bevy copied to clipboard
verified publisher icon bevyengine

Fix API docs for `Commands` methods

Open Nilirad opened this issue 3 years ago • 5 comments

Objective

The doc comments for Command methods are a bit inconsistent on the format, they sometimes go out of scope, and most importantly they are wrong, in the sense that they claim to perform the action described by the command, while in reality, they just push a command to perform the action.

  • Follow-up of #5938.
  • Related to #5913.

Solution

  • Where applicable, only stated that a Command is pushed.
  • Added a “See also” section for similar methods.
  • Added a missing “Panics” section for Commands::entity.
  • Removed a wrong comment about Commands::get_or_spawn returning None (It does not return an option).
  • Removed polluting descriptions of other items.
  • Misc formatting changes.

Future possibilities

Since the Command implementors (Spawn, InsertBundle, InitResource, ...) are public, I thought that it might be appropriate to describe the action of the command there instead of the method, and to add a method → command struct link to fill the gap.

If that seems too far-fetched, we may opt to make them private, if possible, or #[doc(hidden)].

Nilirad avatar Sep 12 '22 09:09 Nilirad

@wilk10 I just added “to the queue”, since “Pushes a Command to the command queue” sounds a little too repetitive.

Nilirad avatar Sep 12 '22 12:09 Nilirad

Tests in code should make explicit what functionality is expected. (Code is law?)

targrub avatar Sep 12 '22 14:09 targrub

Tests in code should make explicit what functionality is expected. (Code is law?)

What do you mean? I'm not touching tests here.

Nilirad avatar Sep 12 '22 14:09 Nilirad

I mean someone ought to write tests for the Commands methods to demonstrate how they ought to work. The documentation can be incorrect/misleading, but having code which demonstrates the intention of how Commands is expected to work is worth even more than solely changing the documentation.

targrub avatar Sep 12 '22 16:09 targrub

For example, if the Commands are actually applied at a certain Stage of the overall ECS system's operation, a set of tests could make that clear. I have no objection to your PR.

targrub avatar Sep 12 '22 16:09 targrub

Pull request successfully merged into main.

Build succeeded:

bors[bot] avatar Sep 21 '22 18:09 bors[bot]