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

Add constants to java gearsbot example #5248

Merged
merged 25 commits into from
Apr 29, 2023

Conversation

bakedPotatoLord
Copy link
Contributor

@bakedPotatoLord bakedPotatoLord commented Apr 1, 2023

Creates a Constants.java file for the java GearsBot example, and implements it everywhere applicable.
There are also a few minor changes including replacing Commands.parallel() with new ParallelCommandGroup() in two instances, and replacing an if gate with a ternary operator in Elevator.java, to improve readability

This fixes half of #1948, but these changes are also required on the C++ example

@bakedPotatoLord bakedPotatoLord requested review from a team as code owners April 1, 2023 20:15
@calcmogul
Copy link
Member

There are also a few minor changes including replacing Commands.parallel() with 'new ParallelCommandGroup()' in two instances, and replacing an if gate with a ternary operator in Elevator.java, to improve readability

Please revert these. The command factories are preferred over allocating commands directly with new. Also, the if statement was fine as it was.

@bakedPotatoLord
Copy link
Contributor Author

The unwanted changes are reverted, and the code is formatted. Awaiting workflow run.

@bakedPotatoLord
Copy link
Contributor Author

Forgot to run wpiformat, but it is formatted now. Awaiting checks. Thank you for your patience.

@bakedPotatoLord
Copy link
Contributor Author

I'm not sure why the windows debug failed. there were no actual code changes since f68a4a4, which passed the check. Can someone please explain what happened?

@rzblue
Copy link
Member

rzblue commented Apr 3, 2023

I went ahead and reran it- likely just a fluke test failure

@bakedPotatoLord
Copy link
Contributor Author

I went ahead and reran it- likely just a fluke test failure

Thank you! Is this ready to merge?

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.

Import the nested *Constants classes directly, to avoid the overly long qualification.

@bakedPotatoLord
Copy link
Contributor Author

Import the nested *Constants classes directly, to avoid the overly long qualification.

fixed by 2b67770. Ready to re-run.

@Starlight220 Starlight220 added the help: needs C++ Java exists, needs C++ port label Apr 6, 2023
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.

Assuming CI passes; looks good.

Now just needs C++.

@prateekma
Copy link
Member

Needs formatting fixes. I am ok with merging this and opening an issue to update C++.

@rzblue
Copy link
Member

rzblue commented Apr 10, 2023

/format

Copy link
Member

@prateekma prateekma 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 open an issue to update this example for C++ when this is merged.

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.

I don't see a reason for a new issue; the one mentioned in the description is enough.

@rzblue
Copy link
Member

rzblue commented Apr 11, 2023

Just need to make sure the issue doesn't get closed when this PR does

@PeterJohnson PeterJohnson merged commit a63d06f into wpilibsuite:main Apr 29, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help: needs C++ Java exists, needs C++ port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants