Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve description of WaitCommand (API doc) #5157

Closed
gartaud opened this issue Mar 2, 2023 · 4 comments · Fixed by #5200
Closed

Improve description of WaitCommand (API doc) #5157

gartaud opened this issue Mar 2, 2023 · 4 comments · Fixed by #5200
Labels
type: docs Related to documentation and information.

Comments

@gartaud
Copy link

gartaud commented Mar 2, 2023

This is a follow up to https://www.chiefdelphi.com/t/best-way-to-implement-custom-timedcommand-in-the-new-aka-2020-command-based-framework/428694/4

The description of WaitCommand at https://github.wpilib.org/allwpilib/docs/release/java/edu/wpi/first/wpilibj2/command/WaitCommand.html
includes the following sentence "Can also be subclassed to make a command with an internal timer."

It does not however warn the user that fully overriding initialize() and end() will break the functionality of the timer which is implemented in initialize() and end(). This unexpected behavior does not seem to be aligned with all the other built-in commands where overriding initialize() and end() is expected.

This suggestion is therefore that initialize() and end() be properly documented at the WaitCommand level and that a warning be added that super().initialize() and super().end() must be called not to break the functionality.

@Starlight220
Copy link
Member

Might be better to just mark the methods as final. Composition should be used rather than inheritance.

@Kevin-OConnor
Copy link
Contributor

I agree we should document this better, especially given that the docs appear to encourage subclassing that command. I will note that contrary to what I see described in the issue, the majority of the built-in command types appear to do things in these methods and will break if you override them without calling super.

@gartaud
Copy link
Author

gartaud commented Mar 2, 2023

Thanks. In retrospect I should have written CommandBase rather than built-in commands. I need to let go of the old framework.

@Starlight220 Starlight220 transferred this issue from wpilibsuite/frc-docs Mar 2, 2023
@Starlight220 Starlight220 added the type: docs Related to documentation and information. label Mar 11, 2023
@Starlight220
Copy link
Member

#5200 removes the note from the API doc.

Furthermore, I'm thinking we should mark all overridden lifecycle methods as final. Though a problem in it would be that it makes implementation changes possibly breaking (if impl overrides another method). Marking the classes themselves as final might be a bit extreme -- constructor-only subclasses are great -- but we do need to ensure people don't accidentally override functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs Related to documentation and information.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants