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

[cmd] Deprecate Command.schedule() #7072

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Sep 12, 2024

It's a footgun and syntactic sugar over the CommandScheduler's schedule method. We don't need syntactic sugar for a footgun.

@Gold856 Gold856 requested a review from a team as a code owner September 12, 2024 11:37
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@Starlight220
Copy link
Member

Usages (such as Trigger) need to be updated

@Gold856 Gold856 requested review from a team as code owners September 17, 2024 23:43
@Gold856 Gold856 force-pushed the deprecate-command-schedule branch from 80686e6 to 8c8d94d Compare September 17, 2024 23:55
@Gold856 Gold856 force-pushed the deprecate-command-schedule branch from 83cac71 to dcc9044 Compare September 18, 2024 06:11
Copy link
Contributor

@spacey-sooty spacey-sooty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

armbot needs update

@Gold856
Copy link
Contributor Author

Gold856 commented Oct 8, 2024

Waiting on #7054 to remove it.

@Gold856 Gold856 force-pushed the deprecate-command-schedule branch 4 times, most recently from 92359e0 to 1a1552f Compare October 12, 2024 22:25
@Gold856 Gold856 force-pushed the deprecate-command-schedule branch 2 times, most recently from 3f6f458 to 8aa7f14 Compare October 24, 2024 13:52
@Daniel1464
Copy link
Contributor

Wait what's the logic over deprecating this again? Scheduling a command in the init block of a class is pretty useful for delayed side effects and such(and having to do CommandScheduler.getInstance().schedule()) is going to be pretty annoying every time this happens

@Gold856
Copy link
Contributor Author

Gold856 commented Oct 28, 2024

The purpose is to be annoying. We've seen time after time again that people will use Command.schedule() and be surprised when their commands get cancelled. The intended way to schedule commands is with Triggers, not Command.schedule(). By forcing the CommandScheduler call, the idea is that people will think more about how their schedule call will affect other commands.

@Daniel1464
Copy link
Contributor

Gotchu; i was just thinking that there should possibly be a utility class for launching side effects(such as SideEffects.waitThenRun(0.4, () -> {...}) to replace the command functionality(although im not sure how much people actually use this). That's probably beyond the scope of this pr though

@Gold856 Gold856 force-pushed the deprecate-command-schedule branch from 8aa7f14 to 1e36eb5 Compare October 29, 2024 06:28
@Gold856 Gold856 force-pushed the deprecate-command-schedule branch from 1e36eb5 to 0739b20 Compare November 8, 2024 18:36
Starlight220
Starlight220 previously approved these changes Dec 17, 2024
Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase, but looks good.

Gold856 and others added 4 commits December 17, 2024 02:24
It's a footgun and syntactic sugar over the CommandScheduler's schedule method. We don't need syntactic sugar for a footgun.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants