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

Improved kotlin support for Commands static factory #7478

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

Conversation

Daniel1464
Copy link

Added an overload of the runOnce and run factories within the Commands namespace that only accepts a lambda argument(without the requirements vararg at the end). Kotlin allows you to "move" lambdas outside of the function body if the lambda is the last argument within the function args, but since it recognizes the vararg as the last argument, you cannot do this for the runOnce and run factories(while being able to for the until() decorator, the waitUntil() factory, and other components such as Triggers).

Before:

Commands.runOnce({
     // Do something here
})

Now:

Commands.runOnce {
   // Do something here
}

@Daniel1464 Daniel1464 requested a review from a team as a code owner December 2, 2024 20:27
Copy link
Contributor

github-actions bot commented Dec 2, 2024

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

@Daniel1464
Copy link
Author

Not applicable to c++/python

@ThadHouse
Copy link
Member

Does this create overload ambiguities? Since there are now 2 functions with the same name that take only 1 parameter?

@Starlight220
Copy link
Member

Starlight220 commented Dec 2, 2024

I'm not a fan of this.

This feels like a way that's too easy to end up with a command without requirements.
The Subsystem.run etc factories already allow this syntax.

Perhaps if we had some Kotlin sources then I'd be all for some sugar such as this and marking it with @JvmSynthetic (=hidden from Java).

Given the ease of extensibility Kotlin has, the potential problems from having this in Java are a cost too high for sugaring Kotlin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants