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

[commands] Clarified error messages for parallel composition commands #7161

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

Conversation

Daniel1464
Copy link

Addresses Issue #6353.

@Daniel1464 Daniel1464 requested a review from a team as a code owner October 4, 2024 00:27
Copy link
Contributor

github-actions bot commented Oct 4, 2024

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

@Daniel1464 Daniel1464 closed this Oct 4, 2024
@Daniel1464
Copy link
Author

Daniel1464 commented Oct 4, 2024

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

robotpy/robotpy-commands-v2#73

@rzblue
Copy link
Member

rzblue commented Oct 4, 2024

/format

@rzblue
Copy link
Member

rzblue commented Oct 4, 2024

You'll need to install and run wpiformat yourself, the PR format command will not work if the PR is based on your main branch. For the future, it's best to make a branch for each PR.

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

We should only include the subsystems that are actually conflicting, since if we just report which ones the added command requires, we aren't actually communicating information that couldn't be found in other ways.

var currentRequirements = getRequirements();
if (!Collections.disjoint(command.getRequirements(), currentRequirements)) {
String requirementsStr = command.getRequirements()
.stream()
Copy link
Member

Choose a reason for hiding this comment

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

The Stream API is really inefficient (for these use cases) and is generally avoided in the WPILib codebase

Copy link
Author

Choose a reason for hiding this comment

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

I feel like it’s probably fine to use the stream api here, even if it’s slow, because this code will only run when an error happens anyways

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.

4 participants