Different Description Of The Command Being Decorated In Documentation And The Hover-Over JavaDocs

[tom131313]

In the documentation of the use of the many command decorators it is not as descriptive as we need for what is the command that is being decorated. I think most or all of the decorators could better describe the command but an especially glaring problem arises when using .andThen() and .withTimeout().

The results of these illustrative snippets are for the most part disconcerting and are significant coding errors made by my team and myself. Especially how can a .withTimeout(1.) on the line for command 3e effect command 1e and command 2e? It's hard to find the error in the code, it takes too much effort to understand why this happens this way, the answer isn't readily remembered and the Javadocs are not helpful when stuck on this problem.

[A circumvention of the problem is to use sequence() but I like .andThen() for its ability to delimit commands better than the , in the sequence( , , ,).]

Stronger clarification of what is the command versus a string of decorators is much needed.

    // this runs command 1a for 1 second
             run(()->System.out.println("command 1a " + Timer.getFPGATimestamp()))
    .andThen(run(()->System.out.println("command 2a " + Timer.getFPGATimestamp()))).withTimeout(1.)
    .schedule();
    
    // this runs command 1b forever
             run(()->System.out.println("command 1b " + Timer.getFPGATimestamp()))
    .andThen(run(()->System.out.println("command 2b " + Timer.getFPGATimestamp())).withTimeout(1.))
    .schedule();

    // runs command 1c for 2 seconds and then runs command 2c for 1 second
             run(()->System.out.println("command 1c " + Timer.getFPGATimestamp())).withTimeout(2.)
    .andThen(run(()->System.out.println("command 2c " + Timer.getFPGATimestamp())).withTimeout(1.))
    .schedule();

    // runs command 1d for 1 second
             run(()->System.out.println("command 1d " + Timer.getFPGATimestamp())).withTimeout(2.)
    .andThen(run(()->System.out.println("command 2d " + Timer.getFPGATimestamp()))).withTimeout(1.)
    .schedule();

    // runs command 1e for 1 second
             run(()->System.out.println("command 1e " + Timer.getFPGATimestamp())).withTimeout(2.)
    .andThen(run(()->System.out.println("command 2e " + Timer.getFPGATimestamp()))).withTimeout(2.)
    .andThen(run(()->System.out.println("command 3e " + Timer.getFPGATimestamp()))).withTimeout(1.)
    .schedule();

[Starlight220]

The case you're bringing up is indeed a mistake that can be easily made with the amount of parentheses involved.

Yet I'm not sure what practical steps can be taken, especially on the library side.
The association of the decorator is whatever expression comes immediately before the .; the challenge is parsing through the parentheses to figure out what exactly that expression is.

Similar pitfalls happen with mathematical order of operations, and API docs have nothing to do with it: it's a question of code readability.

The most helpful solution I've found is to indent blocks commands on the same level; this becomes easier with prefix syntax (= factories) but is possible also in infix/postfix syntax (= decorators).
Most editors/IDEs also have plugins that color matching parentheses; that might help too.

[tom131313]

The association of the decorator is whatever expression comes immediately before the .

Either I misunderstand what you mean or this is not correct - either are the problem I suggest fixing at least with more "inline" documentation (more words, examples) because I, too, can't think of a better syntax to mitigate the problem other than using sequence instead of .andThen().

In my example e the last .withTimeout(1.) is "physically" attached to the immediately preceding
.andThen(run(()->System.out.println("command 3e " + Timer.getFPGATimestamp())))

BUT that is not the command that is modified. The command that receives the timeOut is the first command that is far above - the
run(()->System.out.println("command 1e " + Timer.getFPGATimestamp()))

Yes, the correct placement of that last timeOut for what I meant to do is to the left of one more ) such that it modifies the run command instead of erroneously attempting to modify the andThen which it cannot do but it looks like it should be doing.

[Starlight220]

In my example e the last .withTimeout(1.) is "physically" attached to the immediately preceding
.andThen(run(()->System.out.println("command 3e " + Timer.getFPGATimestamp())))
BUT that is not the command that is modified. The command that receives the timeOut is the first command that is far above - the
run(()->System.out.println("command 1e " + Timer.getFPGATimestamp()))

Note what's going on here:

run(()->System.out.println("command 1e " + Timer.getFPGATimestamp())).withTimeout(2.)
    .andThen(run(()->System.out.println("command 2e " + Timer.getFPGATimestamp()))).withTimeout(2.)
    .andThen(run(()->System.out.println("command 3e " + Timer.getFPGATimestamp()))).withTimeout(1.)
    .schedule();

The expression in the lhs/receiver position of that third withTimeout(1.) is the entire composition.
Each decorator wraps the receiver expression with a composition.

To mirror the math metaphor, the difference is similar to the difference between the following two expressions:

1 * 2 + 3
1 * (2 + 3)

Or in this case with commands:

// Sequence of a and b, with a timeout on the sequence.
a.andThen(b).withTimeout(s)

// Same as before, extract temp to a variable
tmp = a.andThen(b)
tmp.withTimeout(s)

// ---------

// b has a timeout and is added to a sequence after a
a.andThen(b.withTimeout(s))

// Same as previous, extract temp to variable
tmp = b.withTimeout(s)
a.andThen(tmp)

I hope this is clearer.

[tom131313]

Yes, I like your example; this is exactly what I suggest is needed in the methods' Javadocs. Here's a proposed slight change to the existing wording of the withTimeout() Javadoc starting at the "Note:" and includes your examples. I think this will go a long way toward understanding and ease of use for the students. I suggest essentially similar examples for other decorators like .deadlineWith() and andThen().

Note: This decorator works by adding this decorating command to the previous command or composition and creating a new composition. The command composition returned from this method can be further decorated without issue.

// Sequence of a, b, and c with a timeout on the sequence.
a.andThen(b).andThen(c).withTimeout(s)
// same as
sequence(a, b, c).withtimeout(s)

// how it works: each decorator applies to the single previous composition and creates a new composition
tmp1 = a.andThen(b)
tmp2 = tmp1.andThen(c)
tmp3 = tmp2.withTimeout(s)
//-------
// To apply a timeout to only the third portion of the sequence
a.andThen(b).andThen(c.withTimeout(s))
// same as
sequence(a, b, c.withTimeout(s))

The command the decorator was called on cannot be scheduled independently or be added to a different composition (namely, decorators), unless it is manually cleared from the list of composed commands with CommandScheduler.removeComposedCommand(Command).

[Starlight220]

I'm still not seeing how this is an API docs issue - it's a readability and order-of-operations issue.

Adding the full content with an example etc to each decorator's docs would be too verbose, but can be added to an frc-docs page.

We can add a "take note of function call receiver association, see {docs link}" (but phrased better) to the API docs along with the double composition notice?

[tom131313]

Example code in Javadocs is exceedingly rare but it has happened; maybe not this time, though.

Anything that can be done to better document and train users on a couple of idiosyncrasies of compositions is of benefit.

1. This example of the surprising object of the decorator and
2. the surprising case of the default command of a subsystem is not activated within a composition even if there is no currently active use of the subsystem

account for almost all the errors of command-based usage (once the correct command is selected for the problem).